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 4084

Summary: SDL_image.IMG_Load_RW as used by FNA causes heap corruption when called from multiple threads
Product: SDL_image Reporter: Bart van der Werf <bart>
Component: miscAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: flibitijibibo, icculus
Version: unspecified   
Hardware: x86_64   
OS: Windows 7   
Attachments: Minimal reproduction case
C Test Program

Description Bart van der Werf 2018-02-16 14:42:32 UTC 
Created attachment 3171 [details]
Minimal reproduction case

Master version from git@github.com:spurious/SDL-mirror.git most recent commit 10-2-2018 by SamLantinga

https://twitter.com/flibitijibibo suggested i should forward this bug report here.

When using FNA's Texture2D.TextureDataFromStreamEXT which is implemented using SDL_image.IMG_Load_RW in a tight loop on multiple threads concurrently, heap corruption occurs.

https://www.dropbox.com/s/xbp7uuovf8o2k07/PngHeapCorruptionReproductionBartweStaxelPlukitFNA.zip?dl=0

As far as i can tell USE_LOCKS is turned on.


To reproduce run the ConsoleApplication4\ConsoleApplication4\bin\Debug\ConsoleApplication4.exe
Comment 1 Ethan Lee 2018-02-16 16:27:28 UTC 
Created attachment 3173 [details]
C Test Program

I took a look at the test program on Linux and I'm now unsure if that should work anywhere - attached is a C version of the test, with some extras.

When running on Linux without the added SDL_mutex and with more than one thread I get double frees very quickly. With a single thread, or with ADD_MUTEX_SAFETY defined, everything is okay (and it still runs plenty fast).

Where this might vary is with Windows; on Windows this crashes even with ADD_MUTEX_SAFETY defined.

For a quick fix you may just want to use a single thread (doesn't have to be the main thread, I think...), but I'd be interested to know why Windows fails even with locking in place.
Comment 2 Bart van der Werf 2018-02-16 17:14:26 UTC 
If the problem is with the malloc mutex than all work needs to be one the same thread, so it would have to be on the mainthread if you intend to also render things.
Comment 3 Sam Lantinga 2018-02-16 18:21:23 UTC 
I looked at the PNG loader code, and offhand I don't see why it wouldn't be thread-safe. I haven't looked at libpng though. Where does it crash and get double-free'd?
Comment 4 Ethan Lee 2018-02-16 18:28:16 UTC 
Oh weird, I actually got two different results... here's one in IMG_png:

#0  IMG_LoadPNG_RW (src=0x7fffe0000af0) at IMG_png.c:409
        png_num_palette = 32767
        png_palette = 0x8
        start = 0
        error = 0x0
        surface = 0x7fffe00041e0
        png_ptr = 0x7fffe0001b50
        info_ptr = 0x7fffe0002040
        width = 160
        height = 32
        bit_depth = 8
        color_type = 2
        interlace_type = 0
        num_channels = <optimized out>
        Rmask = <optimized out>
        Gmask = <optimized out>
        Bmask = <optimized out>
        Amask = <optimized out>
        palette = 0x925a92925a92925a
        row_pointers = 0x7fffe0007ef0
        row = <optimized out>
        i = <optimized out>
        ckey = <optimized out>
        transv = 0x474e508900000001
#1  0x00007ffff78a0bd6 in IMG_LoadTyped_RW (src=0x7fffe0000af0, freesrc=1, type=0x400b25 "png")
    at IMG.c:195
        i = 8
        image = <optimized out>
#2  0x000000000040093f in Load (data=0x0) at test.c:18
        sur = 0x7fffe00041e0
#3  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0)
    at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
        args = 0x6027c0
        userfunc = 0x400927 <Load>
        userdata = 0x0
        thread = 0x603230
        statusloc = 0x603240
#4  0x00007ffff7b86699 in RunThread (data=<optimized out>)
    at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
No locals.
#5  0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
No symbol table info available.
#6  0x00007ffff75d8e1f in clone () from /lib64/libc.so.6
No symbol table info available.



... and here's a double free:

Thread 8 (Thread 0x7fffe6ffd700 (LWP 18799)):
#0  0x00007ffff7df2a37 in mprotect () from /lib64/ld-linux-x86-64.so.2
#1  0x00007ffff7de2e49 in _dl_relocate_object () from /lib64/ld-linux-x86-64.so.2
#2  0x00007ffff7debba8 in dl_open_worker () from /lib64/ld-linux-x86-64.so.2
#3  0x00007ffff761d18f in _dl_catch_error () from /lib64/libc.so.6
#4  0x00007ffff7deb1f9 in _dl_open () from /lib64/ld-linux-x86-64.so.2
#5  0x00007ffff761c6ad in do_dlopen () from /lib64/libc.so.6
#6  0x00007ffff761d18f in _dl_catch_error () from /lib64/libc.so.6
#7  0x00007ffff761c76c in __libc_dlopen_mode () from /lib64/libc.so.6
#8  0x00007ffff75e80f5 in init () from /lib64/libc.so.6
#9  0x00007ffff6d9e787 in __pthread_once_slow () from /lib64/libpthread.so.0
#10 0x00007ffff75e81cf in backtrace () from /lib64/libc.so.6
#11 0x00007ffff74e7fc9 in backtrace_and_maps () from /lib64/libc.so.6
#12 0x00007ffff7544bac in __libc_message () from /lib64/libc.so.6
#13 0x00007ffff754fa59 in _int_free () from /lib64/libc.so.6
#14 0x00007ffff75553be in free () from /lib64/libc.so.6
#15 0x00007ffff7b2da3f in SDL_free_REAL (ptr=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/stdlib/SDL_malloc.c:5372
#16 0x00007ffff7b5d374 in SDL_FreeFormat_REAL (format=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/video/SDL_pixels.c:620
#17 0x00007ffff7b6377d in SDL_FreeSurface_REAL (surface=0x7fffd40041e0) at /usr/src/debug/SDL2-2.0.7/src/video/SDL_surface.c:1313
#18 0x0000000000400970 in Load (data=0x0) at test.c:20
#19 0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#20 0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#21 0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#22 0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 7 (Thread 0x7fffe77fe700 (LWP 18798)):
#0  0x00007ffff5ffc8ff in inflate () from /lib64/libz.so.1
#1  0x00007ffff6970a0d in png_read_IDAT_data () from /lib64/libpng16.so.16
#2  0x00007ffff696357e in png_read_row () from /lib64/libpng16.so.16
#3  0x00007ffff69651ba in png_read_image () from /lib64/libpng16.so.16
#4  0x00007ffff78a92d4 in IMG_LoadPNG_RW (src=0x7fffd0000af0) at IMG_png.c:385
#5  0x00007ffff78a0bd6 in IMG_LoadTyped_RW (src=0x7fffd0000af0, freesrc=1, type=0x400b25 "png") at IMG.c:195
#6  0x000000000040093f in Load (data=0x0) at test.c:18
#7  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#8  0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#9  0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#10 0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 6 (Thread 0x7fffdf7fe700 (LWP 18797)):
#0  0x00007ffff5ffd1f1 in inflate () from /lib64/libz.so.1
#1  0x00007ffff6970a0d in png_read_IDAT_data () from /lib64/libpng16.so.16
#2  0x00007ffff696357e in png_read_row () from /lib64/libpng16.so.16
#3  0x00007ffff69651ba in png_read_image () from /lib64/libpng16.so.16
#4  0x00007ffff78a92d4 in IMG_LoadPNG_RW (src=0x7fffd8000af0) at IMG_png.c:385
#5  0x00007ffff78a0bd6 in IMG_LoadTyped_RW (src=0x7fffd8000af0, freesrc=1, type=0x400b25 "png") at IMG.c:195
#6  0x000000000040093f in Load (data=0x0) at test.c:18
#7  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#8  0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#9  0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#10 0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 5 (Thread 0x7fffe7fff700 (LWP 18796)):
#0  0x00007ffff5ffd1f1 in inflate () from /lib64/libz.so.1
#1  0x00007ffff6970a0d in png_read_IDAT_data () from /lib64/libpng16.so.16
#2  0x00007ffff696357e in png_read_row () from /lib64/libpng16.so.16
#3  0x00007ffff69651ba in png_read_image () from /lib64/libpng16.so.16
#4  0x00007ffff78a92d4 in IMG_LoadPNG_RW (src=0x7fffe0000af0) at IMG_png.c:385
#5  0x00007ffff78a0bd6 in IMG_LoadTyped_RW (src=0x7fffe0000af0, freesrc=1, type=0x400b25 "png") at IMG.c:195
#6  0x000000000040093f in Load (data=0x0) at test.c:18
#7  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#8  0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#9  0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#10 0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 4 (Thread 0x7ffff4de5700 (LWP 18795)):
#0  0x00007ffff74fe9fb in raise () from /lib64/libc.so.6
#1  0x00007ffff7500800 in abort () from /lib64/libc.so.6
#2  0x00007ffff7544bb1 in __libc_message () from /lib64/libc.so.6
#3  0x00007ffff754fa59 in _int_free () from /lib64/libc.so.6
#4  0x00007ffff75553be in free () from /lib64/libc.so.6
#5  0x00007ffff7b2da3f in SDL_free_REAL (ptr=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/stdlib/SDL_malloc.c:5372
#6  0x00007ffff7b5d374 in SDL_FreeFormat_REAL (format=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/video/SDL_pixels.c:620
#7  0x00007ffff7b6377d in SDL_FreeSurface_REAL (surface=0x7fffec0041e0) at /usr/src/debug/SDL2-2.0.7/src/video/SDL_surface.c:1313
#8  0x0000000000400970 in Load (data=0x0) at test.c:20
#9  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#10 0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#11 0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#12 0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 3 (Thread 0x7ffff55e6700 (LWP 18794)):
#0  0x00007ffff75e771c in __lll_lock_wait_private () from /lib64/libc.so.6
#1  0x00007ffff75e94e4 in __fprintf_chk () from /lib64/libc.so.6
#2  0x00007ffff7ae6635 in SDL_LogMessageV_REAL (category=0, priority=SDL_LOG_PRIORITY_INFO, fmt=<optimized out>, ap=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/SDL_log.c:301
#3  0x00007ffff7af9a64 in SDL_Log (fmt=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/dynapi/SDL_dynapi.c:164
#4  0x0000000000400964 in Load (data=0x0) at test.c:19
#5  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#6  0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#7  0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#8  0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 2 (Thread 0x7ffff5de7700 (LWP 18793)):
#0  0x00007ffff754f87d in _int_free () from /lib64/libc.so.6
#1  0x00007ffff75553be in free () from /lib64/libc.so.6
#2  0x00007ffff7b2da3f in SDL_free_REAL (ptr=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/stdlib/SDL_malloc.c:5372
#3  0x00007ffff7b637c1 in SDL_FreeSurface_REAL (surface=0x7ffff00041e0) at /usr/src/debug/SDL2-2.0.7/src/video/SDL_surface.c:1317
#4  0x0000000000400970 in Load (data=0x0) at test.c:20
#5  0x00007ffff7b2eadc in SDL_RunThread (data=0x6027c0) at /usr/src/debug/SDL2-2.0.7/src/thread/SDL_thread.c:283
#6  0x00007ffff7b86699 in RunThread (data=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_systhread.c:74
#7  0x00007ffff6d9636d in start_thread () from /lib64/libpthread.so.0
#8  0x00007ffff75d8e1f in clone () from /lib64/libc.so.6

Thread 1 (Thread 0x7ffff7fbbb40 (LWP 18789)):
#0  0x00007ffff6d9a61e in pthread_mutex_unlock () from /lib64/libpthread.so.0
#1  0x00007ffff7b86d6e in SDL_UnlockMutex_REAL (mutex=<optimized out>) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_sysmutex.c:184
#2  0x00007ffff7afa6f5 in SDL_PeepEvents_REAL (events=<optimized out>, numevents=<optimized out>, action=SDL_GETEVENT, minType=0, maxType=65535) at /usr/src/debug/SDL2-2.0.7/src/events/SDL_events.c:579
#3  0x00007ffff7afab5f in SDL_WaitEventTimeout_REAL (event=0x7fffffffdb60, timeout=0) at /usr/src/debug/SDL2-2.0.7/src/events/SDL_events.c:682
#4  0x0000000000400a1f in main (argc=1, argv=0x7fffffffdca8) at test.c:48



I'll let Bart dig up some Win32 samples, I'm just checking for crashes via Wine >_>
Comment 5 Ethan Lee 2018-02-16 18:33:54 UTC 
Woops, that's kind of obvious now that I'm debugging with symbols - the SDL_PixelFormat list is not thread-safe:

https://hg.libsdl.org/SDL/file/2088cd828335/src/video/SDL_pixels.c#l592

So there's our double free. Maybe that'll fix the rest too...?
Comment 6 Ryan C. Gordon 2018-02-16 19:56:58 UTC 
Should be fixed in https://hg.libsdl.org/SDL/rev/2a25e8690229, thanks!

--ryan.
Comment 7 Bart van der Werf 2018-02-16 20:23:03 UTC 
Great to see it resolved :)
reproduction doesn't trigger anymore.
A quick look at the surrounding code shows other non threadsafe refcount actions in for example 
SDL_SetPixelFormatPalette
SDL_FreePalette
SDL_InvalidateMap
SDL_MapSurface

or does the contract on those not allow multithreaded access ? or are they non shared resources ?
Comment 8 Ryan C. Gordon 2018-02-17 18:50:32 UTC 
(In reply to Bart van der Werf from comment #7)
> A quick look at the surrounding code shows other non threadsafe refcount
> actions in for example 

Reopening bug to examine.

--ryan.
Comment 9 Ryan C. Gordon 2018-02-17 20:30:45 UTC 
These shouldn't need locks--we only need the lock from the previous patch because multiple threads might step on a global linked list--but they probably should use atomic increments.

I'm not sure if we can safely change these things in the public headers, though (grep for "int refcount")...maybe we can just cast &refcount to (SDL_atomic_t *) internally?

--ryan.
Comment 10 Sam Lantinga 2018-02-18 17:17:18 UTC 
Let's revisit those other functions for a future release.

Thanks!