Skip to content

Avoid recursive pngfix chunk processing - #902

Open
soul-sol wants to merge 1 commit into
pnggroup:libpng18from
soul-sol:fix-pngfix-chunk-recursion
Open

Avoid recursive pngfix chunk processing#902
soul-sol wants to merge 1 commit into
pnggroup:libpng18from
soul-sol:fix-pngfix-chunk-recursion

Conversation

@soul-sol

Copy link
Copy Markdown

Fixes #776.

pngfix relied on the compiler to turn the mutual tail calls between read_chunk() and process_chunk() into jumps. Tail-call elimination is not guaranteed, and sanitizer builds retain a frame for every skipped or continued chunk. A PNG containing many tiny IDAT or ancillary chunks therefore exhausted the process stack.

This change makes read_chunk() own an explicit loop. process_chunk() and sync_stream() now report whether the loop should advance, preserving the existing state transitions without recursion or an arbitrary chunk-count limit.

Reproduction and validation on current libpng18 (d1d0abe):

  • The 3.5 MB tiny-IDAT attachment from [Bug] Protential stack-buffer-overflow crash in pngfix.c #776: baseline ASan stack overflow and exit 134; patched exit 0 with no sanitizer finding.
  • A generated 50,000-chunk tEXt case with --strip=unused: baseline ASan stack overflow and exit 134; patched completes without a sanitizer finding.
  • Normal pngtest.png stdout/stderr are byte-identical before and after the change.
  • ARM64 Ubuntu 24.04 shared/static build and CTest: 37/37 passed.
  • Strict C90 Clang -Wall -Wextra -Werror -pedantic syntax check passed.
  • git diff --check passed.

@jbowler

jbowler commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Tail recursion is standard and has been for at least 50 years (McCarthy created LISP in 1958). Inadequacies in language implementation are not a justification to outlaw or, indeed, condescend, other languages particularly when the code is written in another language; C in this case.

@ctruta; please remove this contribution (i.e. pngfix.c) from libpng, all versions.

@GravisZro

Copy link
Copy Markdown

@jbowler I believe you are being a bit harsh here. What he is presenting is a patch to fix a potential issue, specifically #776 which is a stack overflow. The issue itself is not the use of recursion, it's the use recursion in combination with data from an untrusted source.

@jbowler

jbowler commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

The issue itself is not the use of recursion, it's the use recursion in combination with data from an untrusted source.

Did you repro the example? Let me summarise:

Reproduction and validation on current libpng18 (d1d0abe)

Eh? There's no repro there; there's a test of the fix, there is no repro for the asserted bug, just an analysis. Surely it all passes without the fix?

Perhaps you @GravisZro would care to submit a repro, on that specific commit ID? Please don't use the obvious compiler option.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Protential stack-buffer-overflow crash in pngfix.c

3 participants