LibGfx: Fix PNG decoder leaks from libpng error longjmp

Problem: Runs of the Linux Sanitizers intermittently fail with
LeakSanitizer leaks of ~45 KB across 21 allocations from
PNGLoadingContext::read_frames() — triggered when the test suite
decodes malformed PNGs.

Cause: When libpng hits a corrupted IDAT chunk inside png_read_image(),
it longjmps back to the setjmp landing pad in
PNGLoadingContext::read_all_frames(). longjmp unwinds the stack without
running C++ destructors — so the stack-locals in read_frames
(Vector<u8*> row_pointers, the in-flight Bitmap inside decode_frame, and
the APNG branch’s output buffer and Painter) leak their heap storage.

Fix: Promote those stack-locals to members of PNGLoadingContext (which
is heap-allocated and outlives the setjmp scope) — so their storage is
reachable by RAII when the context is destroyed. Clear them in the
setjmp error handler too — so memory is released promptly on the error
path, rather than waiting until ~PNGLoadingContext().

Author: sideshowbarker

Libraries/LibGfx/ImageFormats/PNGLoader.cpp

@@ -34,13 +34,32 @@ struct PNGLoadingContext {
     Optional<ByteBuffer> icc_profile;
     OwnPtr<ExifMetadata> exif_metadata;
 
+    // Working state for read_frames(). These live as members (rather than as stack-locals inside read_frames()) — so
+    // that a libpng longjmp out of the setjmp scope below doesn't bypass C++ destructors, and leak the heap storage
+    // they own. The setjmp handler clears them, so the memory gets released promptly on the error path.
+    Vector<u8*> row_pointers;
+    RefPtr<Bitmap> in_flight_bitmap;
+    RefPtr<Bitmap> output_buffer;
+    OwnPtr<Painter> painter;
+
+    void clear_read_frames_working_state()
+    {
+        row_pointers.clear();
+        in_flight_bitmap = nullptr;
+        output_buffer = nullptr;
+        painter = nullptr;
+    }
+
     ErrorOr<size_t> read_frames(png_structp, png_infop);
     ErrorOr<void> apply_exif_orientation();
 
     ErrorOr<void> read_all_frames()
     {
         // NOTE: We need to setjmp() here because libpng uses longjmp() for error handling.
         if (auto error_value = setjmp(png_jmpbuf(png_ptr)); error_value) {
+            // longjmp() bypassed the C++ destructors for any stack-locals in read_frames(); release the working-state
+            // members explicitly here — so their heap storage doesn't sit around until ~PNGLoadingContext().
+            clear_read_frames_working_state();
             return Error::from_errno(error_value);
         }
 
@@ -255,25 +274,27 @@ ErrorOr<void> PNGLoadingContext::apply_exif_orientation()
 
 ErrorOr<size_t> PNGLoadingContext::read_frames(png_structp png_ptr, png_infop info_ptr)
 {
-    Vector<u8*> row_pointers;
     auto decode_frame = [&](IntSize frame_size) -> ErrorOr<NonnullRefPtr<Bitmap>> {
-        auto frame_bitmap = TRY(Bitmap::create(BitmapFormat::BGRA8888, AlphaType::Unpremultiplied, frame_size));
+        // Allocate into the in_flight_bitmap member — so the bitmap survives a potential libpng longjmp out of
+        // png_read_image() below.
+        in_flight_bitmap = TRY(Bitmap::create(BitmapFormat::BGRA8888, AlphaType::Unpremultiplied, frame_size));
 
         row_pointers.resize_and_keep_capacity(frame_size.height());
         for (auto i = 0; i < frame_size.height(); ++i)
-            row_pointers[i] = frame_bitmap->scanline_u8(i);
+            row_pointers[i] = in_flight_bitmap->scanline_u8(i);
 
         png_read_image(png_ptr, row_pointers.data());
-        return frame_bitmap;
+        // Past the longjmp window; hand the bitmap to the caller, and clear the member.
+        return in_flight_bitmap.release_nonnull();
     };
 
     if (png_get_acTL(png_ptr, info_ptr, &frame_count, &loop_count)) {
         // acTL chunk present: This is an APNG.
         png_set_acTL(png_ptr, info_ptr, frame_count, loop_count);
 
         // Conceptually, at the beginning of each play the output buffer must be completely initialized to a fully transparent black rectangle, with width and height dimensions from the `IHDR` chunk.
-        auto output_buffer = TRY(Bitmap::create(BitmapFormat::BGRA8888, AlphaType::Unpremultiplied, size));
-        auto painter = Painter::create(output_buffer);
+        output_buffer = TRY(Bitmap::create(BitmapFormat::BGRA8888, AlphaType::Unpremultiplied, size));
+        painter = Painter::create(*output_buffer);
         size_t animation_frame_count = 0;
 
         for (size_t frame_index = 0; frame_index < frame_count; ++frame_index) {
@@ -364,6 +385,9 @@ ErrorOr<size_t> PNGLoadingContext::read_frames(png_structp png_ptr, png_infop in
         auto decoded_frame_bitmap = TRY(decode_frame(size));
         frame_descriptors.append({ move(decoded_frame_bitmap), 0 });
     }
+
+    // Clear the working state — so it doesn't outlive read_frames() on the success path.
+    clear_read_frames_working_state();
     return frame_count;
 }
 

Tests/LibGfx/TestImageDecoder.cpp

@@ -516,6 +516,19 @@ TEST_CASE(test_png_malformed_frame)
     }
 }
 
+// Regression test: libpng longjmp() out of png_read_image() must not leak the in-flight row-pointers buffer or bitmap
+// from PNGLoadingContext::read_frames(). The fixture has a valid IHDR, but a corrupted IDAT body that decompresses past
+// its zlib end-of-block marker — forcing libpng to longjmp mid-decode.
+TEST_CASE(test_png_corrupt_idat_does_not_leak_on_libpng_longjmp)
+{
+    auto file = TRY_OR_FAIL(Core::MappedFile::map(TEST_INPUT("png/corrupt-idat.png"sv)));
+    auto plugin_or_error = Gfx::PNGImageDecoderPlugin::create(file->bytes());
+    // PNGImageDecoderPlugin::create() catches the libpng error and falls back to a single-frame placeholder — so we
+    // don't assert on the result here. The test passes if the run completes without LeakSanitizer reports.
+    if (!plugin_or_error.is_error())
+        (void)plugin_or_error.release_value()->frame(0);
+}
+
 TEST_CASE(test_png_large_dimensions)
 {
     auto file = TRY_OR_FAIL(Core::MappedFile::map(TEST_INPUT("png/65535x1.png"sv)));