We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4614

Summary: [PATCH] Use-after-free in Cocoa video backend after SDL_DestroyWindow
Product: SDL Reporter: Cameron Gutman <cameron.gutman>
Component: videoAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: major    
Priority: P2 CC: icculus
Version: HG 2.1Keywords: target-2.0.10
Hardware: x86   
OS: macOS 10.13   
Attachments: Patch to clear contentView
Full ASan output

Description Cameron Gutman 2019-04-29 00:50:36 UTC 
Created attachment 3770 [details]
Patch to clear contentView

AddressSanitizer found the following use-after-free (full context attached):
==95569==ERROR: AddressSanitizer: heap-use-after-free on address 0x611001408a80 at pc 0x000100d9ee7a bp 0x7ffeefbfbb80 sp 0x7ffeefbfbb78
READ of size 8 at 0x611001408a80 thread T0
2019-04-27 19:20:38.514018-0700 atos[95573:710920] examining /Users/USER/*/Moonlight.app/Contents/MacOS/Moonlight [95569]
2019-04-27 19:20:38.916726-0700 atos[95575:710936] examining /Users/USER/*/Moonlight.app/Contents/MacOS/Moonlight [95569]
    #0 0x100d9ee79 in -[SDLView updateLayer] SDL_cocoawindow.m:1193
    #1 0x7fff3a7f1954 in _NSViewUpdateLayer (AppKit:x86_64+0x125954)
    #2 0x7fff3a7f136e in -[_NSViewBackingLayer display] (AppKit:x86_64+0x12536e)
    #3 0x7fff47aabd1c in CA::Layer::display_if_needed(CA::Transaction*) (QuartzCore:x86_64+0x14d1c)
    #4 0x7fff47a99f41 in CA::Context::commit_transaction(CA::Transaction*) (QuartzCore:x86_64+0x2f41)
    #5 0x7fff47a99589 in CA::Transaction::commit() (QuartzCore:x86_64+0x2589)
    #6 0x7fff3a7e80a0 in __65+[CATransaction(NSCATransaction) NS_setFlushesWithDisplayRefresh]_block_invoke (AppKit:x86_64+0x11c0a0)
    #7 0x7fff3d0bde87 in __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ (CoreFoundation:x86_64h+0x97e87)
    #8 0x7fff3d0bddbc in __CFRunLoopDoObservers (CoreFoundation:x86_64h+0x97dbc)
    #9 0x7fff3d0604cf in __CFRunLoopRun (CoreFoundation:x86_64h+0x3a4cf)
    #10 0x7fff3d05fe0d in CFRunLoopRunSpecific (CoreFoundation:x86_64h+0x39e0d)
    #11 0x7fff3c34c9da in RunCurrentEventLoopInMode (HIToolbox:x86_64+0xa9da)
    #12 0x7fff3c34c61c in ReceiveNextEventCommon (HIToolbox:x86_64+0xa61c)
    #13 0x7fff3c34c4a5 in _BlockUntilNextEventMatchingListInModeWithFilter (HIToolbox:x86_64+0xa4a5)
    #14 0x7fff3a6e6ffa in _DPSNextEvent (AppKit:x86_64+0x1affa)
    #15 0x7fff3a6e5d92 in -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (AppKit:x86_64+0x19d92)
    #16 0x7fff3a6dfeaf in -[NSApplication run] (AppKit:x86_64+0x13eaf)
    #17 0x109318cca in QCocoaEventDispatcher::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (libqcocoa.dylib:x86_64+0x33cca)
    #18 0x102b0150e in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (QtCore:x86_64+0x1e450e)
    #19 0x102b06451 in QCoreApplication::exec() (QtCore:x86_64+0x1e9451)
    #20 0x10000b67f in main main.cpp:470
    #21 0x7fff694d83d4 in start (libdyld.dylib:x86_64+0x163d4)

0x611001408a80 is located 192 bytes inside of 216-byte region [0x6110014089c0,0x611001408a98)
freed by thread T0 here:
    #0 0x102f6220d in wrap_free (libclang_rt.asan_osx_dynamic.dylib:x86_64h+0x5c20d)
    #1 0x100d401c8 in SDL_free_REAL SDL_malloc.c:5431
    #2 0x10112fede in SDL_DestroyWindow_REAL SDL_video.c:2758
    #3 0x10128f803 in SDL_DestroyWindow SDL_dynapi_procs.h:577
    #4 0x10010a7da in Session::exec(int, int) session.cpp:1256
    #5 0x100174323 in Session::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) moc_session.cpp:112
    #6 0x1001757e4 in Session::qt_metacall(QMetaObject::Call, int, void**) moc_session.cpp:199


The problem is that the contentView is still around while the window is being asynchronously closed. If we receive an updateLayer callback before the window is destroyed, it will access the freed SDL_WindowData memory. Clearing contentView prior to freeing SDL_WindowData makes the use-after-free go away.

I believe this is the real cause behind https://bugzilla.libsdl.org/show_bug.cgi?id=4394 and https://bugzilla.libsdl.org/show_bug.cgi?id=4386
Comment 1 Cameron Gutman 2019-04-29 00:52:11 UTC 
Created attachment 3771 [details]
Full ASan output
Comment 2 Ryan C. Gordon 2019-05-18 18:48:54 UTC 
Tagging a bunch of bugs with "target-2.0.10" so we have a clear list of things to address before a 2.0.10 release.

Please note that "addressing" one of these bugs might mean deciding to defer on it until after 2.0.10, or resolving it as WONTFIX, etc. This is just here to tell us we should look at it carefully, and soon.

If you have new information or feedback on this issue, this is a good time to add it to the conversation, as we're likely to be paying attention to this specific report in the next few days/weeks.

Thanks!

--ryan.
Comment 3 Ryan C. Gordon 2019-05-21 04:18:53 UTC 
This patch is now https://hg.libsdl.org/SDL/rev/132a2af7edac, thanks!

--ryan.