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

Tries to free previous results of SDL_SetVideoMode(), leading to use-after-free crash #305

Open
smcv opened this issue Jul 18, 2023 · 1 comment · May be fixed by #306
Open

Tries to free previous results of SDL_SetVideoMode(), leading to use-after-free crash #305

smcv opened this issue Jul 18, 2023 · 1 comment · May be fixed by #306

Comments

@smcv
Copy link

smcv commented Jul 18, 2023

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
@smcv smcv mentioned this issue Jul 18, 2023
Closed
@smcv
Copy link
Author

smcv commented Jul 18, 2023

libsdl-org/sdl12-compat#305 contains C code that is a simplification of what this binding is doing, leading to the same crash.

Here's a similarly cut-down version of core_video.t: core_video.txt

smcv added a commit to smcv/SDL1_perl that referenced this issue Jul 18, 2023
@smcv
Distinguish between owned and borrowed references to a SDL_Surface
e9b907c 
In many SDL APIs that return a SDL_Surface *, the surface is considered
to be owned by the caller, and must be freed by the caller.

However, SDL_SetVideoMode and presumably SDL_GetVideoSurface return
a pointer to SDL's internal video surface, which will be freed by SDL
if necessary, and must not be freed by library users.
Incorrectly freeing this surface can lead to a use-after-free crash,
manifesting as a test failure in t/core_video.t.

See also libsdl-org/sdl12-compat#305

Resolves: PerlGameDev#305
Signed-off-by: Simon McVittie <[email protected]>
This was referenced Jul 18, 2023
Open
Open
smcv added a commit to smcv/sdl12-compat that referenced this issue Jul 20, 2023
@smcv
Allocate video surface object statically as a global
468d89a 
The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result
of functions that return a "borrowed" pointer to the video surface,
namely SDL_SetVideoMode() and SDL_GetVideoSurface().
(See PerlGameDev/SDL#305)

When we would previously have allocated or freed the video surface
wrapper object, instead allocate or free its contents in-place.

When checking whether the video surface exists, because we never destroy
it, we must now also check whether its underlying SDL2 video surface
exists.

Resolves: libsdl-org#305
Signed-off-by: Simon McVittie <[email protected]>
@smcv smcv mentioned this issue Jul 20, 2023
Merged
smcv added a commit to smcv/sdl12-compat that referenced this issue Aug 26, 2023
@smcv
Allocate video surface object statically as a global
7717c26 
The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result
of functions that return a "borrowed" pointer to the video surface,
namely SDL_SetVideoMode() and SDL_GetVideoSurface().
(See PerlGameDev/SDL#305)

When we would previously have allocated or freed the video surface
wrapper object, instead allocate or free its contents in-place.

When checking whether the video surface exists, because we never destroy
it, we must now also check whether its underlying SDL2 video surface
exists.

Resolves: libsdl-org#305
Signed-off-by: Simon McVittie <[email protected]>
slouken pushed a commit to libsdl-org/sdl12-compat that referenced this issue Aug 26, 2023
@smcv @slouken
Allocate video surface object statically as a global
d86a531 
The SDL Perl bindings incorrectly call SDL_FreeSurface() on the result
of functions that return a "borrowed" pointer to the video surface,
namely SDL_SetVideoMode() and SDL_GetVideoSurface().
(See PerlGameDev/SDL#305)

When we would previously have allocated or freed the video surface
wrapper object, instead allocate or free its contents in-place.

When checking whether the video surface exists, because we never destroy
it, we must now also check whether its underlying SDL2 video surface
exists.

Resolves: #305
Signed-off-by: Simon McVittie <[email protected]>
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

Successfully merging a pull request may close this issue.

Distinguish between owned and borrowed references to a SDL_Surface smcv/SDL1_perl
1 participant
@smcv