Skip to content

Incorrect pixel output from png_image_read_direct_scaled() for interlaced 16-bit to 8-bit conversion #853

Description

@quguanni

Summary

png_image_read_direct_scaled() produces incorrect pixel values when decoding Adam7-interlaced 16-bit PNGs to 8-bit output via the simplified API (png_image_finish_read). Approximately 50% of pixel channels are wrong, with errors up to 214/255.

This is a correctness bug, not a memory safety issue — all reads and writes remain within allocated bounds.

Root Cause

The function uses local_row as an intermediate buffer. For interlaced images, png_read_row() calls png_combine_row() internally, which reads existing content in the row buffer to merge new pass data with pixels from previous passes. However, local_row contains stale data from the previous row of the current pass rather than the accumulated output from previous passes — that data was copied to output_row but never restored into local_row for the next pass.

Introduced in commit 218612d ("Rearchitect the fix to the buffer overflow in png_image_finish_read").

Trigger Conditions

All three must be true:

  1. Input PNG uses Adam7 interlacing
  2. Input has 16-bit depth
  3. Decoded via png_image_finish_read() requesting 8-bit output (i.e., PNG_FORMAT_FLAG_LINEAR not set)

This triggers the do_local_scale path. Non-interlaced images, 8-bit inputs, and the traditional API are unaffected.

Reproducer

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "png.h"

int main(int argc, char **argv)
{
    if (argc < 3) {
        fprintf(stderr, "Usage: %s <interlaced_16bit.png> <flat_16bit.png>\n", argv[0]);
        return 1;
    }

    png_image img_il = {0}, img_flat = {0};
    img_il.version = PNG_IMAGE_VERSION;
    img_flat.version = PNG_IMAGE_VERSION;

    if (!png_image_begin_read_from_file(&img_il, argv[1])) return 1;
    img_il.format = PNG_FORMAT_RGB;
    size_t size_il = PNG_IMAGE_SIZE(img_il);
    png_byte *buf_il = malloc(size_il);
    if (!png_image_finish_read(&img_il, NULL, buf_il,
            (png_int_32)PNG_IMAGE_ROW_STRIDE(img_il), NULL)) return 1;

    if (!png_image_begin_read_from_file(&img_flat, argv[2])) return 1;
    img_flat.format = PNG_FORMAT_RGB;
    size_t size_flat = PNG_IMAGE_SIZE(img_flat);
    png_byte *buf_flat = malloc(size_flat);
    if (!png_image_finish_read(&img_flat, NULL, buf_flat,
            (png_int_32)PNG_IMAGE_ROW_STRIDE(img_flat), NULL)) return 1;

    int diffs = 0, max_diff = 0;
    for (size_t i = 0; i < size_il && i < size_flat; i++) {
        int d = abs((int)buf_il[i] - (int)buf_flat[i]);
        if (d > 0) { diffs++; if (d > max_diff) max_diff = d; }
    }

    printf("Differing channels: %d / %zu, max diff: %d\n", diffs, size_il, max_diff);
    free(buf_il); free(buf_flat);
    return diffs > 0 ? 2 : 0;
}

To generate test PNGs, use any tool that can save the same pixel data as both interlaced and non-interlaced 16-bit RGB PNG. For example with Python/Pillow:

from PIL import Image
import struct, zlib, io

def write_png_16bit(w, h, pixels, interlaced, path):
    """Write a 16-bit RGB PNG manually (Pillow doesn't support 16-bit write)."""
    def chunk(ctype, data):
        c = ctype + data
        return struct.pack('>I', len(data)) + c + struct.pack('>I', zlib.crc32(c) & 0xffffffff)

    with open(path, 'wb') as f:
        f.write(b'\x89PNG\r\n\x1a\n')
        f.write(chunk(b'IHDR', struct.pack('>IIBBBBB', w, h, 16, 2, 0, 0, 1 if interlaced else 0)))
        raw = b''
        for y in range(h):
            raw += b'\x00'  # filter none
            for x in range(w):
                r, g, b = pixels[y * w + x]
                raw += struct.pack('>HHH', r, g, b)
        f.write(chunk(b'IDAT', zlib.compress(raw)))
        f.write(chunk(b'IEND', b''))

# 16x16 gradient
pixels = [(x * 4096, y * 4096, (x + y) * 2048) for y in range(16) for x in range(16)]
write_png_16bit(16, 16, pixels, True, 'test_interlaced.png')
write_png_16bit(16, 16, pixels, False, 'test_flat.png')

Output (before fix)

MISBEHAVIOR: pixel (0,0) ch=0: interlaced=112 flat=0 diff=112
MISBEHAVIOR: pixel (0,0) ch=1: interlaced=189 flat=119 diff=70
MISBEHAVIOR: pixel (0,0) ch=2: interlaced=152 flat=234 diff=82
...
Total differing channels: 384 / 768
Max difference: 214

Proposed Fix

Restore accumulated output into local_row before png_read_row() on passes after the first, so png_combine_row() merges against the correct pixel data:

-   while (--passes >= 0)
    {
-      png_uint_32 y = image->height;
-      png_byte *output_row = first_row;
+      int pass = 0;
 
-      for (; y > 0; --y)
+      while (--passes >= 0)
       {
-         png_read_row(png_ptr, local_row, NULL);
+         png_uint_32 y = image->height;
+         png_byte *output_row = first_row;
+
+         for (; y > 0; --y)
+         {
+            if (pass > 0)
+               memcpy(local_row, output_row, row_bytes);
 
-         memcpy(output_row, local_row, row_bytes);
-         output_row += row_step;
+            png_read_row(png_ptr, local_row, NULL);
+
+            memcpy(output_row, local_row, row_bytes);
+            output_row += row_step;
+         }
+
+         ++pass;
       }
    }

Full git format-patch is attached / available on request.

All 36 cmake tests pass with this fix, and the reproducer confirms interlaced and non-interlaced outputs now match exactly.

Affected Versions

Any version containing commit 218612d ("Rearchitect the fix to the buffer overflow in png_image_finish_read") — libpng 1.6.51+ and libpng 1.8.x development branch.

Image [poc_005_interlace_misbehavior.c](https://github.com/user-attachments/files/27031362/poc_005_interlace_misbehavior.c) Image

0001-pngread-fix-incorrect-interlace-combining-in-png_ima.patch

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions