Use-after-free in SDL_DestroyRenderer when SDL_DestroyWindow is called first

[takase1121]

Valgrind reported the following:

==13877== Memcheck, a memory error detector
==13877== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==13877== Using Valgrind-3.23.0 and LibVEX; rerun with -h for copyright info
==13877== Command: ./lite-xl
==13877==
==13877== Invalid read of size 8
==13877==    at 0x49E7BD2: SDL_SetWindowData_REAL (SDL_video.c:2127)
==13877==    by 0x49115CB: UnknownInlinedFun (SDL_render.c:4357)
==13877==    by 0x49115CB: SDL_DestroyRenderer_REAL (SDL_render.c:4321)
==13877==    by 0x124500: renwin_free (renwindow.c:139)
==13877==    by 0x123C63: ren_destroy (renderer.c:576)
==13877==    by 0x115A4A: f_renwin_gc (renwindow.c:63)
==13877==    by 0x133BC4: precallC (ldo.c:529)
==13877==    by 0x133EE7: luaD_precall (ldo.c:595)
==13877==    by 0x134123: ccall (ldo.c:635)
==13877==    by 0x1341C0: luaD_callnoyield (ldo.c:655)
==13877==    by 0x13832C: dothecall (lgc.c:901)
==13877==    by 0x132B20: luaD_rawrunprotected (ldo.c:144)
==13877==    by 0x134A8D: luaD_pcall (ldo.c:953)
==13877==  Address 0x6952750 is 0 bytes inside a block of size 240 free'd
==13877==    at 0x484797F: free (vg_replace_malloc.c:989)
==13877==    by 0x49ECD50: UnknownInlinedFun (SDL_malloc.c:5339)
==13877==    by 0x49ECD50: SDL_DestroyWindow_REAL.part.0 (SDL_video.c:3350)
==13877==    by 0x1244D5: renwin_free (renwindow.c:135)
==13877==    by 0x123C63: ren_destroy (renderer.c:576)
==13877==    by 0x115A4A: f_renwin_gc (renwindow.c:63)
==13877==    by 0x133BC4: precallC (ldo.c:529)
==13877==    by 0x133EE7: luaD_precall (ldo.c:595)
==13877==    by 0x134123: ccall (ldo.c:635)
==13877==    by 0x1341C0: luaD_callnoyield (ldo.c:655)
==13877==    by 0x13832C: dothecall (lgc.c:901)
==13877==    by 0x132B20: luaD_rawrunprotected (ldo.c:144)
==13877==    by 0x134A8D: luaD_pcall (ldo.c:953)
==13877==  Block was alloc'd at
==13877==    at 0x484BF03: calloc (vg_replace_malloc.c:1675)
==13877==    by 0x49E4F8A: UnknownInlinedFun (SDL_malloc.c:5311)
==13877==    by 0x49E4F8A: SDL_CreateWindow_REAL (SDL_video.c:1724)
==13877==    by 0x11596C: f_renwin_create (renwindow.c:43)
==13877==    by 0x133BC4: precallC (ldo.c:529)
==13877==    by 0x133EE7: luaD_precall (ldo.c:595)
==13877==    by 0x15CEC9: luaV_execute (lvm.c:1684)
==13877==    by 0x13414B: ccall (ldo.c:637)
==13877==    by 0x1341C0: luaD_callnoyield (ldo.c:655)
==13877==    by 0x128EE0: f_call (lapi.c:1038)
==13877==    by 0x132B20: luaD_rawrunprotected (ldo.c:144)
==13877==    by 0x134A8D: luaD_pcall (ldo.c:953)
==13877==    by 0x128FB8: lua_pcallk (lapi.c:1064)
==13877==
==13877== Invalid read of size 8
==13877==    at 0x4941D20: UnknownInlinedFun (SDL_video.c:2836)
==13877==    by 0x4941D20: SW_DestroyRenderer.lto_priv.0 (SDL_render_sw.c:987)
==13877==    by 0x124500: renwin_free (renwindow.c:139)
==13877==    by 0x123C63: ren_destroy (renderer.c:576)
==13877==    by 0x115A4A: f_renwin_gc (renwindow.c:63)
==13877==    by 0x133BC4: precallC (ldo.c:529)
==13877==    by 0x133EE7: luaD_precall (ldo.c:595)
==13877==    by 0x134123: ccall (ldo.c:635)
==13877==    by 0x1341C0: luaD_callnoyield (ldo.c:655)
==13877==    by 0x13832C: dothecall (lgc.c:901)
==13877==    by 0x132B20: luaD_rawrunprotected (ldo.c:144)
==13877==    by 0x134A8D: luaD_pcall (ldo.c:953)
==13877==    by 0x1384C1: GCTM (lgc.c:921)
==13877==  Address 0x6952750 is 0 bytes inside a block of size 240 free'd
==13877==    at 0x484797F: free (vg_replace_malloc.c:989)
==13877==    by 0x49ECD50: UnknownInlinedFun (SDL_malloc.c:5339)
==13877==    by 0x49ECD50: SDL_DestroyWindow_REAL.part.0 (SDL_video.c:3350)
==13877==    by 0x1244D5: renwin_free (renwindow.c:135)
==13877==    by 0x123C63: ren_destroy (renderer.c:576)
==13877==    by 0x115A4A: f_renwin_gc (renwindow.c:63)
==13877==    by 0x133BC4: precallC (ldo.c:529)
==13877==    by 0x133EE7: luaD_precall (ldo.c:595)
==13877==    by 0x134123: ccall (ldo.c:635)
==13877==    by 0x1341C0: luaD_callnoyield (ldo.c:655)
==13877==    by 0x13832C: dothecall (lgc.c:901)
==13877==    by 0x132B20: luaD_rawrunprotected (ldo.c:144)
==13877==    by 0x134A8D: luaD_pcall (ldo.c:953)
==13877==  Block was alloc'd at
==13877==    at 0x484BF03: calloc (vg_replace_malloc.c:1675)
==13877==    by 0x49E4F8A: UnknownInlinedFun (SDL_malloc.c:5311)
==13877==    by 0x49E4F8A: SDL_CreateWindow_REAL (SDL_video.c:1724)
==13877==    by 0x11596C: f_renwin_create (renwindow.c:43)
==13877==    by 0x133BC4: precallC (ldo.c:529)
==13877==    by 0x133EE7: luaD_precall (ldo.c:595)
==13877==    by 0x15CEC9: luaV_execute (lvm.c:1684)
==13877==    by 0x13414B: ccall (ldo.c:637)
==13877==    by 0x1341C0: luaD_callnoyield (ldo.c:655)
==13877==    by 0x128EE0: f_call (lapi.c:1038)
==13877==    by 0x132B20: luaD_rawrunprotected (ldo.c:144)
==13877==    by 0x134A8D: luaD_pcall (ldo.c:953)
==13877==    by 0x128FB8: lua_pcallk (lapi.c:1064)
==13877==
==13877==
==13877== HEAP SUMMARY:
==13877==     in use at exit: 377,497 bytes in 3,294 blocks
==13877==   total heap usage: 230,034 allocs, 226,740 frees, 102,996,877 bytes allocated
==13877==
==13877== LEAK SUMMARY:
==13877==    definitely lost: 1,560 bytes in 22 blocks
==13877==    indirectly lost: 1,885 bytes in 12 blocks
==13877==      possibly lost: 0 bytes in 0 blocks
==13877==    still reachable: 373,972 bytes in 3,258 blocks
==13877==         suppressed: 80 bytes in 2 blocks
==13877== Rerun with --leak-check=full to see details of leaked memory
==13877==
==13877== For lists of detected and suppressed errors, rerun with: -s
==13877== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

This is a build of lite-xl/lite-xl@96579d9 with SDL2 2.30.3. Looking at

SDL/src/render/SDL_render.c

Line 4356 in fb14975

if (renderer->window) {

it does seem like SDL2 tries to access renderer->window, which would be invalid after SDL_DestroyWindow. #9574 supposedly fixed this in SDL3 but

SDL/src/render/SDL_render.c

Line 4604 in 9d47dae

if (renderer->window) {

seems to do a similar thing (I can be wrong, SDL_ClearProperty may check if the pointer is valid, in that case this error would not exist).

If this is the intended behavior please clarify it so that I could fix this in Lite XL.

[icculus]

#9574 supposedly fixed this in SDL3 but

That function sets a flag that makes this work, but it would definitely be clearer to set renderer->window to NULL, too. I've now done this, in 9f8dffb.

I don't think we ever cherry-picked the rest of the fixes back to SDL2. Let me figure out why real fast.

[icculus]

Me in the original bug report: "This is fixed in SDL3; reopened to look at SDL2 branch, hopefully tomorrow."

I guess I fully intended to do this, so now's as good a time as any!

But, to be clear: if you want this to work on older versions of SDL2, you need to destroy the renderer first and then the window. This change I'm looking to pull into SDL2 makes it safe to do it in either order, but since people will need a bleeding edge SDL2, you should probably change the order you destroy these things in your program.

[icculus]

Okay, latest in the SDL2 branch has the SDL3 fixes cherry-picked to it, so this bug is resolved for future SDL2 releases (but again, you probably should switch the order you destroy this stuff, for people running older SDL2 releases).

[takase1121]

No problem, I just hoped that this would be elaborated in the docs a bit, especially in the docs for SDL_DestroyRenderer and SDL_DestroyWindow.

Another side effect that I've observed is that SDL_Texture is also destroyed when the renderer is destroyed, so it might also be worth documenting.

[slouken]

Another side effect that I've observed is that SDL_Texture is also destroyed when the renderer is destroyed, so it might also be worth documenting.

It is documented, in the main description of the function:

SDL/include/SDL3/SDL_render.h

Line 2010 in 8412f52

* Destroy the rendering context for a window and free all associated textures.