Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SDL_image.IMG_Load_RW as used by FNA causes heap corruption when called from multiple threads #126

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

SDL_image.IMG_Load_RW as used by FNA causes heap corruption when called from multiple threads #126

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: unspecified
Reported for operating system, platform: Windows 7, x86_64

Comments on the original bug report:

On 2018-02-16 14:42:32 +0000, Bart van der Werf wrote:

Created attachment 3171
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

On 2018-02-16 16:27:28 +0000, Ethan Lee wrote:

Created attachment 3173
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.

On 2018-02-16 17:14:26 +0000, Bart van der Werf wrote:

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.

On 2018-02-16 18:21:23 +0000, Sam Lantinga wrote:

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?

On 2018-02-16 18:28:16 +0000, Ethan Lee wrote:

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=)

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=) at /usr/src/debug/SDL2-2.0.7/src/stdlib/SDL_malloc.c:5372

16 0x00007ffff7b5d374 in SDL_FreeFormat_REAL (format=) 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=) 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=) 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=) 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=) 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=) at /usr/src/debug/SDL2-2.0.7/src/stdlib/SDL_malloc.c:5372

6 0x00007ffff7b5d374 in SDL_FreeFormat_REAL (format=) 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=) 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=, ap=) at /usr/src/debug/SDL2-2.0.7/src/SDL_log.c:301

3 0x00007ffff7af9a64 in SDL_Log (fmt=) 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=) 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=) 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=) 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=) at /usr/src/debug/SDL2-2.0.7/src/thread/pthread/SDL_sysmutex.c:184

2 0x00007ffff7afa6f5 in SDL_PeepEvents_REAL (events=, numevents=, 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 >_>

On 2018-02-16 18:33:54 +0000, Ethan Lee wrote:

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...?

On 2018-02-16 19:56:58 +0000, Ryan C. Gordon wrote:

Should be fixed in https://hg.libsdl.org/SDL/rev/2a25e8690229, thanks!

--ryan.

On 2018-02-16 20:23:03 +0000, Bart van der Werf wrote:

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 ?

On 2018-02-17 18:50:32 +0000, Ryan C. Gordon wrote:

(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.

On 2018-02-17 20:30:45 +0000, Ryan C. Gordon wrote:

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.

On 2018-02-18 17:17:18 +0000, Sam Lantinga wrote:

Let's revisit those other functions for a future release.

Thanks!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@SDLBugzilla