[Git main] SDL_Quit does not free the surface created by SDL_SetVideoMode but should?

[hartwork]

Hi!

The SDL 1.2 docs on SDL_SetVideoMode say that:

The surface returned is freed by SDL_Quit() and should not be freed by the caller.

I noticed that this doesn't seem to hold true for sdl12-compat SDL_Quit, i.e. AddressSanitizer shows a leak with this minimal reproducer:

// main.c
#include <SDL.h>

int main() {
  SDL_Init(SDL_INIT_EVERYTHING);
  SDL_Surface *const window = SDL_SetVideoMode(640, 480, 32, SDL_RESIZABLE);
  SDL_Quit();
  return 0;
}

This is with latest f52d7787f9f513a5a2ba99d92678af39a9548458 and I'm running the code like this (without need for any Makefile present):

# make main \
    CFLAGS="$(pkg-config sdl12_compat --cflags) -g -fsanitize=address -fno-omit-frame-pointer" \
    LDFLAGS="$(pkg-config sdl12_compat --libs) -g -fsanitize=address -fno-omit-frame-pointer" \
  && ./main

The runtime output is this:

=================================================================
==27777==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 88 byte(s) in 1 object(s) allocated from:
    #0 0x7f7bc26e6937 in __interceptor_malloc [..]/gcc-11-20221223/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x7f7bbeba66e3 in SDL_malloc_REAL [..]/sdl2/src/stdlib/SDL_malloc.c:5292
    #2 0x7f7bc25d7dc9 in SDL_CreateRGBSurface [..]/sdl12-compat/src/SDL12_compat.c:5060
    #3 0x7f7bc25e45f4 in CreateSurface12WithFormat [..]/sdl12-compat/src/SDL12_compat.c:5479
    #4 0x7f7bc25e45f4 in SetVideoModeImpl [..]/sdl12-compat/src/SDL12_compat.c:6054
    #5 0x7f7bc25e45f4 in SDL_SetVideoMode [..]/sdl12-compat/src/SDL12_compat.c:6253
    #6 0x5629522481af in main /tmp/tmp.xK0Oc03Hap/main.c:5
    #7 0x7f7bc23fd209  (/lib64/libc.so.6+0x2a209)

Indirect leak of 224 byte(s) in 2 object(s) allocated from:
    #0 0x7f7bc26e6aff in __interceptor_calloc [..]/gcc-11-20221223/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x7f7bbeba6721 in SDL_calloc_REAL [..]/sdl2/src/stdlib/SDL_malloc.c:5308

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f7bc26e6937 in __interceptor_malloc [..]/gcc-11-20221223/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x7f7bbeba66e3 in SDL_malloc_REAL [..]/sdl2/src/stdlib/SDL_malloc.c:5292
    #2 0x7f7bc25d7dc9 in SDL_CreateRGBSurface [..]/sdl12-compat/src/SDL12_compat.c:5060
    #3 0x7f7bc25e45f4 in CreateSurface12WithFormat [..]/sdl12-compat/src/SDL12_compat.c:5479
    #4 0x7f7bc25e45f4 in SetVideoModeImpl [..]/sdl12-compat/src/SDL12_compat.c:6054
    #5 0x7f7bc25e45f4 in SDL_SetVideoMode [..]/sdl12-compat/src/SDL12_compat.c:6253
    #6 0x5629522481af in main /tmp/tmp.xK0Oc03Hap/main.c:5
    #7 0x7f7bc23fd209  (/lib64/libc.so.6+0x2a209)

SUMMARY: AddressSanitizer: 360 byte(s) leaked in 4 allocation(s).

Adding SDL_FreeSurface(window); after (not before) SDL_Quit(); fixes the leak but also seems evil since SDL is already officially shut down and it's not needed with SDL 1.2. What do you think?

Best, Sebastian

[icculus]

Ok, we cleaned up most of the window, but there was a small memory leak because SDL_FreeSurface() refuses to free the screen surface's memory...so now we just make sure to NULL out the global that holds the screen surface pointer before attempting to free the object during SDL_Quit.

Good catch on this one, thanks!

[hartwork]

@icculus I confirm that db10d6fc01a2a567ddf58e9890b3bbeb6fa5c77c fixed the leak, awesome! :+1: