Title: GifMakeSavedImage: stale ImageCount use-after-free and OOB write in the
free/append lifecycle; self-aliased copy UAF; NULL-handling hardening
Severity: memory corruption (heap OOB write, heap use-after-free)
Version: 6.1.3 / current head (a8e3114)
Labels: security, memory-safety
=== Bug 1: stale ImageCount after GifFreeSavedImages (heap OOB write) ===
GifFreeSavedImages() frees GifFile->SavedImages and NULLs the pointer
but leaves ImageCount at its old value. A subsequent
GifMakeSavedImage() allocates a fresh one-slot array and indexes it
with the stale count (gifalloc.c:345), so for an N-frame GIF the
memset()/memcpy() writes a full SavedImage N slots past the end of a
1-slot allocation. Reachable from the documented public-API sequence
DGifSlurp(); GifFreeSavedImages(); GifMakeSavedImage(...);
i.e. any client that releases frame memory and keeps decoding (stream
and animation decoders). AddressSanitizer: "heap-buffer-overflow,
WRITE of size 56" in GifMakeSavedImage at gifalloc.c:424 (memset),
0 bytes after a 56-byte region allocated at gifalloc.c:332.
=== Bug 2: self-aliased copy (heap use-after-free) ===
GifMakeSavedImage(gf, &gf->SavedImages[i]) - the natural way to
duplicate a frame inside one GifFile - reallocarray()s the array
(gifalloc.c:338, freeing the old block) before memcpy()-ing the
source record (gifalloc.c:348): the struct copy reads through the
dangling pointer. ASan: "heap-use-after-free, READ of size 56",
freed by reallocarray in the same call frame. Copies across two
GifFile handles are unaffected, which is why no in-tree caller trips
this (gifsponge/giftool/gifbuild all copy across handles).
=== Patches attached: 0004, 0005, 0006, 0008 ===
0004-gifalloc-keep-ImageCount-consistent-in-GifFreeSavedI.patch
Resets ImageCount to 0 when the array is dropped - exactly what
the maintainer-added DGifDecreaseImageCounter() error path already
does (dgif_lib.c:1166-1171), and what GifFreeExtensions() does for
its own count.
0005-gifalloc-snapshot-CopyFrom-before-growing-SavedImage.patch
Takes a stack snapshot of *CopyFrom before the realloc, extending
the aliasing guard the function's own comment already claims.
0006-gifalloc-harden-NULL-handling-in-the-image-extension.patch
(a) GifMakeSavedImage CopyFrom path: a blank image (NULL
RasterBits) copied with dimensions set passed NULL to memcpy()
(UBSan nonnull violation + SEGV); now zero-fills instead, so
copies never contain uninitialized bytes either. (b)
FreeLastSavedImage() NULLs RasterBits after free, as it already
does for ColorMap. (c) GifAddExtensionBlock() rolls back the
speculative count when the Bytes allocation fails, so a failed
add is not observable as a phantom block.
0008-tests-add-regression-tests-for-the-memory-safety-fix.patch
Adds tests/poc-regress (TAP, hooked into tapstream): 18 checks
locking all of the above in as permanent regressions, plus the
quantize/encoder/constructor checks from my other tickets.
All fixes verified: make check 69 tests 0 failures; the PoCs above go
from ASan aborts to clean runs; byte-identical output for the tools on
the upstream test GIFs.
Correction — please use these attachments instead of the earlier 0004–0006.
The previously attached patches contained leftover merge-conflict markers in the gifalloc hunks — a commit-tooling error on my side, caught during follow-up work. The corrected series (6 patches, all final-state, no fixup-on-fixup) supersedes them:
0004 (attached): gifalloc memory-safety hardening as one commit with all four fixes and full evidence: (1) stale ImageCount after GifFreeSavedImages() → OOB write in GifMakeSavedImage(); (2) self-aliased copy → heap-use-after-free (realloc invalidates the source); (3) NULL-raster copy, dangling RasterBits, speculative extension count; (4) ExtensionBlockCount never restored by the deep copy — copies lose GCEs (frame timing/disposal/transparency) and leak the copied Bytes (this also retires my earlier OOM-aliasing worry: the maintainer's null-out already protects the source). The count tracks completed blocks per iteration, so every exit frees exactly what was built; the failure path was additionally exercised with -Wl,--wrap=malloc under valgrind (0 errors, source intact — glibc-specific, run locally).
0005 (attached): DGifOpen/EGifOpen reject NULL I/O callbacks, fail closed.
0006 (attached): tests/api_test.c + makefile wiring — 23 checks covering the series (GCE fixture included so the extension preservation check genuinely bites), build rule tracks library sources and honors $(CFLAGS), tests clean target, binary gitignored.
Plus two fixes filed as their own tickets with their own checks: quantize *ColorMapSize bounds (new ticket), encoder color-map representability → CodeMask overread (new ticket).
Verified: full suite 74 tests, 0 failures; api_test 23/23 under ASan+UBSan; all patches marker-free; each fix's before/after sanitizer evidence in the original ticket text above. Apologies for the churn — the fixes themselves are unchanged in substance; the packaging is what's corrected.
Last edit: Mohmamed Mohamed Elnady 2026-09-30