stb icon indicating copy to clipboard operation
stb copied to clipboard

Fixes the first stbi_load_gif issue in #1838 by clearing `delays` on all outofmem paths

Open NBickford-NV opened this issue 6 months ago • 2 comments

After calling stbi_load_gif, apps should free the delays pointer it passed in if delays is non-null. This means that after freeing delays in the stbi__load_gif_main_outofmem error path, stb_image_load should also set delays to NULL. Otherwise if we allocate delays once but we get to stbi__load_gif_main_outofmem on a later frame, then the app sees a non-null delays pointer and tries to free it again.

With this change, running clang++ poc.c -fsanitize=address && ./a.out 490442704-000e388f-3edd-409b-9253-83046ac80317.gif from #1838 now correctly reports "Failed to load GIF" instead of segfaulting.

Forks with JarLob's double-free fix in #1549 were also affected; it implemented this logic on some paths, but not all, and #1838 happens to reach one of the other paths. Moving the delays = NULL logic to stbi__load_gif_main_outofmem on those forks allows us to save a few lines of code.

In addition, since realloc first frees the input pointer (C specification 7.22.3.5 item 1), we shouldn't pass the input to stbi__load_gif_main_outofmem after a realloc -- then we'd get a different double-free. If we remove the temporary pointer and do p = realloc(p, ...) then I think we get the right behavior and save a few lines of code.

Thanks!

NBickford-NV avatar Sep 18 '25 01:09 NBickford-NV

My colleague Pyarelal Knowles noticed that I made a mistake in simplifying the calls to realloc in stbi__load_gif_main and accidentally added a memory leak; if realloc returns NULL because it failed, then C specification 7.22.3.5 item 3 says it doesn't free the input pointer:

If memory for the new object cannot be allocated, the old object is not deallocated and its value is unchanged.

I've reverted my changes to stbi__load_gif_main to the previous version, which I think was correct, so now the only change is we clear delays in outofmem. Tagging @sezero since this PR was integrated into SDL_image in https://github.com/libsdl-org/SDL_image/commit/658e4423772cef1b552432547691e08387002dd9 .

NBickford-NV avatar Dec 05 '25 00:12 NBickford-NV

Tagging @sezero since this PR was integrated into SDL_image in libsdl-org/SDL_image@658e442 .

Updated SDL repos, thanks.

sezero avatar Dec 05 '25 00:12 sezero