Tries to free previous results of SDL_SetVideoMode(), leading to use-after-free crash #305
Description
This binding is packaged in Debian as libsdl-perl. Since trying to switch from classic SDL 1.2 to sdl12-compat in Debian, we've been seeing crashes in its test perl t/core_video.t. On 32-bit platforms it seems to segfault repeatably; on 64-bit platforms it usually succeeds, but using valgrind or AddressSanitizer reveals that there is still a use-after-free, it's just not fatal for whatever reason.
I asked SDL/sdl12-compat upstream about this, and they say that calling SDL_FreeSurface() on the result of SDL_SetVideoMode() is considered to be a programming error (invalid/undefined behaviour), because the video surface is "borrowed" from SDL internals. This is mitigated by SDL silently ignoring attempts to free the current video surface; however, the test keeps a pointer to an older video surface around, and tries to free that later, after SDL has already freed it.
I think the correct solution is likely to be for this binding to distinguish between SDL_Surface objects that are "owned" by the caller (which it should free), and SDL_Surface objects that are "borrowed" from SDL (which it must not free). At the moment, the binding assumes that it "owns" all SDL_Surface objects returned by any SDL API.
If I understand correctly, SDL_GetVideoSurface() has similar memory-management, and therefore its Perl interface has a similar bug.
Here's an example of the use-after-free, using versions of SDL2 and sdl12-compat that have been compiled using AddressSanitizer:
AddressSanitizer output
$ LD_PRELOAD=libasan.so.8 ASAN_OPTIONS=detect_leaks=0 LD_LIBRARY_PATH="$HOME/tmp/build/SDL2/asan/build/.libs:$HOME/tmp/build/sdl12-compat/asan" make -f debian/rules build
...
ok 63 - '[get_video_surface] Checking if we get a surface ref back' isa 'SDL::Surface'
ok 64 - [video_driver_name] This is your driver name: dummy
ok 65 - [video_mode_ok] Checking if an integer was return
=================================================================
==523156==ERROR: AddressSanitizer: heap-use-after-free on address 0x608000062374 at pc 0x7ff9d7705152 bp 0x7ffc0daa9aa0 sp 0x7ffc0daa9a98
READ of size 4 at 0x608000062374 thread T0
#0 0x7ff9d7705151 in SDL_FreeSurface /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5182
#1 0x7ff9d991904f in objDESTROY src/helper.h:65
#2 0x7ff9d991904f in objDESTROY src/helper.h:52
#3 0x7ff9d9919102 in XS_SDL__Surface_DESTROY lib/SDL/Surface.xs:185
#4 0x5566edd73f17 in Perl_pp_entersub (/usr/bin/perl+0x123f17) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#5 0x5566edcc01c4 in Perl_call_sv (/usr/bin/perl+0x701c4) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#6 0x5566edd81cfb (/usr/bin/perl+0x131cfb) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#7 0x5566edd8244f in Perl_sv_clear (/usr/bin/perl+0x13244f) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#8 0x5566edd82a2f in Perl_sv_free2 (/usr/bin/perl+0x132a2f) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#9 0x5566edd6b2e8 in Perl_pp_sassign (/usr/bin/perl+0x11b2e8) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#10 0x5566edd69ef5 in Perl_runops_standard (/usr/bin/perl+0x119ef5) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#11 0x5566edcc8778 in perl_run (/usr/bin/perl+0x78778) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#12 0x5566edc9a4b1 in main (/usr/bin/perl+0x4a4b1) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#13 0x7ff9da3666c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#14 0x7ff9da366784 in __libc_start_main_impl ../csu/libc-start.c:360
#15 0x5566edc9a4f0 in _start (/usr/bin/perl+0x4a4f0) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
0x608000062374 is located 84 bytes inside of 88-byte region [0x608000062320,0x608000062378)
freed by thread T0 here:
#0 0x7ff9da6d7288 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
#1 0x7ff9d5949a02 in real_free /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5199
#2 0x7ff9d5949eaa in SDL_free_REAL /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5339
#3 0x7ff9d5754036 in SDL_free /home/smcv/src/SDL-2.x/src/dynapi/SDL_dynapi_procs.h:411
#4 0x7ff9d7705119 in SDL_FreeSurface /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5190
#5 0x7ff9d77053c7 in EndVidModeCreate /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5508
#6 0x7ff9d7712c60 in SetVideoModeImpl /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5982
#7 0x7ff9d771516f in SDL_SetVideoMode /home/smcv/src/sdl12-compat/src/SDL12_compat.c:6329
#8 0x7ff9d97dd60e in XS_SDL__Video_set_video_mode lib/SDL/Video.xs:137
previously allocated by thread T0 here:
#0 0x7ff9da6d85bf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x7ff9d5949a23 in real_malloc /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5196
#2 0x7ff9d5949de1 in SDL_malloc_REAL /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5295
#3 0x7ff9d5754012 in SDL_malloc /home/smcv/src/SDL-2.x/src/dynapi/SDL_dynapi_procs.h:408
#4 0x7ff9d76d55af in Surface20to12 /home/smcv/src/sdl12-compat/src/SDL12_compat.c:4932
#5 0x7ff9d7704b9b in SDL_CreateRGBSurface /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5130
#6 0x7ff9d7704e0d in CreateSurface12WithFormat /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5551
#7 0x7ff9d77136b2 in SetVideoModeImpl /home/smcv/src/sdl12-compat/src/SDL12_compat.c:6126
#8 0x7ff9d771516f in SDL_SetVideoMode /home/smcv/src/sdl12-compat/src/SDL12_compat.c:6329
#9 0x7ff9d97dd60e in XS_SDL__Video_set_video_mode lib/SDL/Video.xs:137
SUMMARY: AddressSanitizer: heap-use-after-free /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5182 in SDL_FreeSurface
Shadow bytes around the buggy address:
0x608000062080: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x608000062100: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x608000062180: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x608000062200: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
0x608000062280: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
=>0x608000062300: fa fa fa fa fd fd fd fd fd fd fd fd fd fd[fd]fa
0x608000062380: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x608000062400: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x608000062480: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x608000062500: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
0x608000062580: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==523156==ABORTING
Dubious, test returned 1 (wstat 256, 0x100)
All 65 subtests passed
(2 TODO tests unexpectedly succeeded)
t/core_palette.t shows a similar user-after-free, although we haven't observed this causing real-world crashes in Debian (even on 32-bit) for whatever reason:
AddressSanitizer output
ok 8 - 'Palette->colors is an array' isa 'ARRAY'
ok 9 - 'Palette->colors[x] is an SDL::Color' isa 'SDL::Color'
ok 10 - 'Palette->color_index() is a SDL::Color' isa 'SDL::Color'
=================================================================
==523072==ERROR: AddressSanitizer: heap-use-after-free on address 0x60800005f8f4 at pc 0x7f92fb505152 bp 0x7ffcb9ab82d0 sp 0x7ffcb9ab82c8
READ of size 4 at 0x60800005f8f4 thread T0
#0 0x7f92fb505151 in SDL_FreeSurface /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5182
#1 0x7f92fd70504f in objDESTROY src/helper.h:65
#2 0x7f92fd70504f in objDESTROY src/helper.h:52
#3 0x7f92fd705102 in XS_SDL__Surface_DESTROY lib/SDL/Surface.xs:185
#4 0x56087a8abf17 in Perl_pp_entersub (/usr/bin/perl+0x123f17) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#5 0x56087a7f81c4 in Perl_call_sv (/usr/bin/perl+0x701c4) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#6 0x56087a8b9cfb (/usr/bin/perl+0x131cfb) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#7 0x56087a8ba44f in Perl_sv_clear (/usr/bin/perl+0x13244f) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#8 0x56087a8baa2f in Perl_sv_free2 (/usr/bin/perl+0x132a2f) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#9 0x56087a8e7eef in Perl_leave_scope (/usr/bin/perl+0x15feef) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#10 0x56087a8f55d2 in Perl_pp_leave (/usr/bin/perl+0x16d5d2) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#11 0x56087a8a1ef5 in Perl_runops_standard (/usr/bin/perl+0x119ef5) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#12 0x56087a80067e in perl_run (/usr/bin/perl+0x7867e) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#13 0x56087a7d24b1 in main (/usr/bin/perl+0x4a4b1) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
#14 0x7f92fe1666c9 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#15 0x7f92fe166784 in __libc_start_main_impl ../csu/libc-start.c:360
#16 0x56087a7d24f0 in _start (/usr/bin/perl+0x4a4f0) (BuildId: 42daa0cc03328cecf85c1f8589ec1619f547c3a5)
0x60800005f8f4 is located 84 bytes inside of 88-byte region [0x60800005f8a0,0x60800005f8f8)
freed by thread T0 here:
#0 0x7f92fe4d7288 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
#1 0x7f92f9749a02 in real_free /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5199
#2 0x7f92f9749eaa in SDL_free_REAL /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5339
#3 0x7f92f9554036 in SDL_free /home/smcv/src/SDL-2.x/src/dynapi/SDL_dynapi_procs.h:411
#4 0x7f92fb505119 in SDL_FreeSurface /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5190
#5 0x7f92fb5053c7 in EndVidModeCreate /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5508
#6 0x7f92fb512c60 in SetVideoModeImpl /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5982
#7 0x7f92fb51516f in SDL_SetVideoMode /home/smcv/src/sdl12-compat/src/SDL12_compat.c:6329
#8 0x7f92fd5f860e in XS_SDL__Video_set_video_mode lib/SDL/Video.xs:137
previously allocated by thread T0 here:
#0 0x7f92fe4d85bf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
#1 0x7f92f9749a23 in real_malloc /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5196
#2 0x7f92f9749de1 in SDL_malloc_REAL /home/smcv/src/SDL-2.x/src/stdlib/SDL_malloc.c:5295
#3 0x7f92f9554012 in SDL_malloc /home/smcv/src/SDL-2.x/src/dynapi/SDL_dynapi_procs.h:408
#4 0x7f92fb4d55af in Surface20to12 /home/smcv/src/sdl12-compat/src/SDL12_compat.c:4932
#5 0x7f92fb504b9b in SDL_CreateRGBSurface /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5130
#6 0x7f92fb504e0d in CreateSurface12WithFormat /home/smcv/src/sdl12-compat/src/SDL12_compat.c:5551
#7 0x7f92fb5136b2 in SetVideoModeImpl /home/smcv/src/sdl12-compat/src/SDL12_compat.c:6126
#8 0x7f92fb51516f in SDL_SetVideoMode /home/smcv/src/sdl12-compat/src/SDL12_compat.c:6329
#9 0x7f92fd5f860e in XS_SDL__Video_set_video_mode lib/SDL/Video.xs:137