Hi Sam,
it seems that there a double free corruption on iOS when uiscreenmodes
are released: here is the bt
Hedgewars(4502,0xa0c16540) malloc: *** error for object 0x5e42240:
pointer being freed was not allocated
#0 0x99cc4c5a in __kill ()
#1 0x99cc4c4c in kill$UNIX2003 ()
#2 0x99d575a5 in raise ()
#3 0x99d6d6e4 in abort ()
#4 0x99c6a575 in free ()
#5 0x00273012 in UIKit_ReleaseUIScreenMode (mode=0x5a49e00) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/uikit/SDL_uikitvideo.m:353
#6 0x002730a5 in UIKit_VideoQuit (_this=0x6849c00) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/uikit/SDL_uikitvideo.m:370
#7 0x0026f1fa in SDL_VideoQuit () at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/SDL_video.c:2078
#8 0x002218f9 in SDL_QuitSubSystem (flags=65535) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/SDL.c:186
#9 0x00221978 in SDL_Quit () at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/SDL.c:214
#10 0x00002971 in ONDESTROY () at hwengine.s:352
#11 0x00003cdb in GAME (GAMEARGS=0xbfffded8) at hwengine.s:1626
#12 0x000f2527 in -[GameInterfaceBridge engineLaunch] (self=0x5c72770,
_cmd=0x2ec9de) at
/Users/vittorio/hedgewars/trunk/project_files/HedgewarsMobile/Classes/GameInterfaceBridge.m:177
#13 0x007487f6 in __NSFireDelayedPerform ()
#14 0x01c8bfe3 in CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION ()
#15 0x01c8d594 in __CFRunLoopDoTimer ()
#16 0x01be9cc9 in __CFRunLoopRun ()
#17 0x01be9240 in CFRunLoopRunSpecific ()
#18 0x01be9161 in CFRunLoopRunInMode ()
#19 0x0239f268 in GSEventRunModal ()
#20 0x0239f32d in GSEventRun ()
#21 0x00c7442e in UIApplicationMain ()
#22 0x002756e4 in main (argc=1, argv=0xbfffef18) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/uikit/SDL_uikitappdelegate.m:59
what has struck me is that in UIKit_VideoQuit there are two calls to
UIKit_ReleaseUIScreenMode
UIKit_ReleaseUIScreenMode(&display->desktop_mode);
UIKit_ReleaseUIScreenMode(&display->current_mode);
and by debugging it appears that for iOS both current_mode and
desktop_mode point to the same object.
Is it normal to have duplicate pointers? If so why releasing twice?
I naively tried removing one of the calls and the crash disappears,
but could you look at this, as it seems to be some dangerous area?
Thanks,
Vittorio
I filed a bug report about this ages ago, but your naive solution is fine
if you’re only dealing with a single screen mode. I think that whenever
current_mode is acquired it needs to be retained, but I’m going to look
into the code right now to see what’s up.On Tue, Feb 7, 2012 at 8:32 AM, Vittorio Giovara <vitto.giova at yahoo.it>wrote:
Hi Sam,
it seems that there a double free corruption on iOS when uiscreenmodes
are released: here is the bt
Hedgewars(4502,0xa0c16540) malloc: *** error for object 0x5e42240:
pointer being freed was not allocated
#0 0x99cc4c5a in __kill ()
#1 0x99cc4c4c in kill$UNIX2003 ()
#2 0x99d575a5 in raise ()
#3 0x99d6d6e4 in abort ()
#4 0x99c6a575 in free ()
#5 0x00273012 in UIKit_ReleaseUIScreenMode (mode=0x5a49e00) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/uikit/SDL_uikitvideo.m:353
#6 0x002730a5 in UIKit_VideoQuit (_this=0x6849c00) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/uikit/SDL_uikitvideo.m:370
#7 0x0026f1fa in SDL_VideoQuit () at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/SDL_video.c:2078
#8 0x002218f9 in SDL_QuitSubSystem (flags=65535) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/SDL.c:186
#9 0x00221978 in SDL_Quit () at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/SDL.c:214
#10 0x00002971 in ONDESTROY () at hwengine.s:352
#11 0x00003cdb in GAME (GAMEARGS=0xbfffded8) at hwengine.s:1626
#12 0x000f2527 in -[GameInterfaceBridge engineLaunch] (self=0x5c72770,
_cmd=0x2ec9de) at
/Users/vittorio/hedgewars/trunk/project_files/HedgewarsMobile/Classes/GameInterfaceBridge.m:177
#13 0x007487f6 in __NSFireDelayedPerform ()
#14 0x01c8bfe3 in
CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION ()
#15 0x01c8d594 in __CFRunLoopDoTimer ()
#16 0x01be9cc9 in __CFRunLoopRun ()
#17 0x01be9240 in CFRunLoopRunSpecific ()
#18 0x01be9161 in CFRunLoopRunInMode ()
#19 0x0239f268 in GSEventRunModal ()
#20 0x0239f32d in GSEventRun ()
#21 0x00c7442e in UIApplicationMain ()
#22 0x002756e4 in main (argc=1, argv=0xbfffef18) at
/Users/vittorio/hedgewars/Library/SDL/Xcode-iOS/SDL/…/…/src/video/uikit/SDL_uikitappdelegate.m:59
what has struck me is that in UIKit_VideoQuit there are two calls to
UIKit_ReleaseUIScreenMode
UIKit_ReleaseUIScreenMode(&display->desktop_mode);
UIKit_ReleaseUIScreenMode(&display->current_mode);
and by debugging it appears that for iOS both current_mode and
desktop_mode point to the same object.
Is it normal to have duplicate pointers? If so why releasing twice?
I naively tried removing one of the calls and the crash disappears,
but could you look at this, as it seems to be some dangerous area?
Thanks,
Vittorio
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
Ah good catch. It’s interesting that I didn’t see this when I was
working on the Retina display support. I changed stuff around here a bit
and while I didn’t change the semantics there is a subtle gotcha because
of the disconnect between Obj-C reference counting and C explicit memory
management. The fix is to guard the free with a check for the
retainCount of the uiscreenmode. i.e.:
diff -r a2082bc91789 SDL/src/video/uikit/SDL_uikitvideo.m
— a/SDL/src/video/uikit/SDL_uikitvideo.m Wed Jan 25 17:08:37 2012 +0000
+++ b/SDL/src/video/uikit/SDL_uikitvideo.m Tue Feb 07 10:24:14 2012 +0000
@@ -350,7 +350,9 @@
} else {
SDL_DisplayModeData *data = (SDL_DisplayModeData
*)mode->driverdata;
[data->uiscreenmode release];
I don’t really have time to work on this today so I haven’t tested this,
but that should do the trick. Sorry about the bug ;).
P.S. It could be argued that the real bug is that UIKit_AddDisplay
should create new copies of mode when assigning to display.desktop_mode
and display.current_mode and thus avoid the icky double retain on mode.
I’ll probably have a look at this when I get a chance…On 07/02/2012 00:32, Vittorio Giovara wrote:
it seems that there a double free corruption on iOS when uiscreenmodes
are released: here is the bt
what has struck me is that in UIKit_VideoQuit there are two calls to
UIKit_ReleaseUIScreenMode
UIKit_ReleaseUIScreenMode(&display->desktop_mode);
UIKit_ReleaseUIScreenMode(&display->current_mode);
and by debugging it appears that for iOS both current_mode and
desktop_mode point to the same object.
Is it normal to have duplicate pointers? If so why releasing twice?
I naively tried removing one of the calls and the crash disappears,
but could you look at this, as it seems to be some dangerous area?
Jeremy, can you provide a link for the bugreport you mentioned?
Tim, are you sure using retainCount is a good approach? As I was
reading some documentation on this method, it seems that it can never
return 0, so the SDL_free will never happen. More on this topic here
http://www.friday.com/bbum/2011/12/18/retaincount-is-useless/
VittorioOn Tue, Feb 7, 2012 at 11:30 AM, Tim Angus wrote:
On 07/02/2012 00:32, Vittorio Giovara wrote:
it seems that there a double free corruption on iOS when uiscreenmodes
are released: here is the bt
what has struck me is that in UIKit_VideoQuit there are two calls to
UIKit_ReleaseUIScreenMode
? ? ? ? UIKit_ReleaseUIScreenMode(&display->desktop_mode);
? ? ? ? UIKit_ReleaseUIScreenMode(&display->current_mode);
and by debugging it appears that for iOS both current_mode and
desktop_mode point to the same object.
Is it normal to have duplicate pointers? If so why releasing twice?
I naively tried removing one of the calls and the crash disappears,
but could you look at this, as it seems to be some dangerous area?
Ah good catch. It’s interesting that I didn’t see this when I was working on
the Retina display support. I changed stuff around here a bit and while I
didn’t change the semantics there is a subtle gotcha because of the
disconnect between Obj-C reference counting and C explicit memory
management. The fix is to guard the free with a check for the retainCount of
the uiscreenmode. i.e.:
diff -r a2082bc91789 SDL/src/video/uikit/SDL_uikitvideo.m
— a/SDL/src/video/uikit/SDL_uikitvideo.m ? ? ?Wed Jan 25 17:08:37 2012
+0000
+++ b/SDL/src/video/uikit/SDL_uikitvideo.m ? ? ?Tue Feb 07 10:24:14 2012
+0000
@@ -350,7 +350,9 @@
? ? } else {
? ? ? ? SDL_DisplayModeData *data = (SDL_DisplayModeData *)mode->driverdata;
? ? ? ? [data->uiscreenmode release];
- ? ? ? ?if ([data->uiscreenmode retainCount] == 0) {
- ? ? ? ? ? ?SDL_free(data);
- ? ? ? ?}
? ? ? ? mode->driverdata = NULL;
? ? }
?}
I don’t really have time to work on this today so I haven’t tested this, but
that should do the trick. Sorry about the bug ;).
P.S. It could be argued that the real bug is that UIKit_AddDisplay should
create new copies of mode when assigning to display.desktop_mode and
display.current_mode and thus avoid the icky double retain on mode. I’ll
probably have a look at this when I get a chance…
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org
Jeremy, can you provide a link for the bugreport you mentioned?
Tim, are you sure using retainCount is a good approach? As I was
reading some documentation on this method, it seems that it can never
return 0, so the SDL_free will never happen. More on this topic here
http://www.friday.com/bbum/2011/12/18/retaincount-is-useless/
Heh, that won’t work then. This might though:
SDL_DisplayModeData *data = (SDL_DisplayModeData
*)mode->driverdata;
BOOL freedata = NO;
if ([data->uiscreenmode retainCount] == 1) {
freedata = YES;
}
[data->uiscreenmode release];
if (freedata) {
SDL_free(data);
}
mode->driverdata = NULL;
As I said before though the real fix is probably as follows:On 07/02/2012 14:38, Vittorio Giovara wrote:
P.S. It could be argued that the real bug is that UIKit_AddDisplay should
create new copies of mode when assigning to display.desktop_mode and
display.current_mode and thus avoid the icky double retain on mode. I’ll
probably have a look at this when I get a chance…
I can’t find the bug report, I think I actually just posted on the forum
right after Sam left for a little while. Anyways, I’m sure I did mention
the bug, and I even have it patched in my own sources, but what is really
weird is that the 2.0 source I just downloaded looks like there was an
attempt to prevent the double release because of the following lines in
UIKit_AddDisplay:
[data->uiscreenmode retain]; // once for the desktop_mode
[data->uiscreenmode retain]; // once for the current_mode
Further on down in UIKit_VideoQuit there is:
UIKit_ReleaseUIScreenMode(&display->desktop_mode);
UIKit_ReleaseUIScreenMode(&display->current_mode);
for (j = 0; j < display->num_display_modes; j++) {
SDL_DisplayMode *mode = &display->display_modes[j];
UIKit_ReleaseUIScreenMode(mode);
}
From what I see here it looks like there are potentially 3 releases but I
haven’t really figured out how the display->display_modes array is
populated. For the record, I’m using a very old patched 1.3 in my iOS game
and haven’t been bitten by this double release, however, I was being hit by
it before I manually patched the source. I’ll try and figure out the
deal-ee-yo when I can squeeze some time in.
OK, I had a little chance to have a peek at this. Having had a closer
look at things, I believe the actual problem is that current_mode should
not be retained or owned at all. If you look at the rest of SDL it’s
used as a reference to other modes rather than a mode in its own right.
It would probably help get its semantics clear if its type was actually
a pointer rather than a value i.e.:
struct SDL_VideoDisplay
{
int max_display_modes;
int num_display_modes;
SDL_DisplayMode *display_modes;
SDL_DisplayMode desktop_mode;
- SDL_DisplayMode current_mode;
The attached patch contains a bit of extra noise to centralise the
allocation of SDL_DisplayModeData, but in essence the fix is just to not
retain, allocate or free current_mode, and treat it as a weak reference
only. I haven’t really had time to properly test this so YMMV, but I
think this is the correct route to take.
HTH.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed…
Name: sdl2-ios-double-free-fix.diff
URL: http://lists.libsdl.org/pipermail/sdl-libsdl.org/attachments/20120207/c4cf9033/attachment.txtOn 07/02/2012 14:47, Tim Angus wrote:
As I said before though the real fix is probably as follows:
P.S. It could be argued that the real bug is that UIKit_AddDisplay
should
create new copies of mode when assigning to display.desktop_mode and
display.current_mode and thus avoid the icky double retain on mode. I’ll
probably have a look at this when I get a chance…
I’ve been really busy recently, but looking over the patch I wrote it
seems to do the correct thing. Did anybody having troubles here try the
patch and did it help? I’ll probably stick it in bugzilla if there
aren’t any issues with it.On 07/02/2012 18:37, Tim Angus wrote:
The attached patch contains a bit of extra noise to centralise the
allocation of SDL_DisplayModeData, but in essence the fix is just to not
retain, allocate or free current_mode, and treat it as a weak reference
only. I haven’t really had time to properly test this so YMMV, but I
think this is the correct route to take.
Hi Tim, I meant to check the patch sooner but I got distracted as well.
I tested the patch now on simulator and device and the double free
seems actually fixed.
So bugzilla it is
thanks
VittorioOn Fri, Feb 17, 2012 at 6:25 PM, Tim Angus wrote:
On 07/02/2012 18:37, Tim Angus wrote:
The attached patch contains a bit of extra noise to centralise the
allocation of SDL_DisplayModeData, but in essence the fix is just to not
retain, allocate or free current_mode, and treat it as a weak reference
only. I haven’t really had time to properly test this so YMMV, but I
think this is the correct route to take.
I’ve been really busy recently, but looking over the patch I wrote it seems
to do the correct thing. Did anybody having troubles here try the patch and
did it help? I’ll probably stick it in bugzilla if there aren’t any issues
with it.
SDL mailing list
SDL at lists.libsdl.org
http://lists.libsdl.org/listinfo.cgi/sdl-libsdl.org