Skip to content

fix(image): charge GIF frames at the canvas size Pillow decodes them at - #4526

Open
badGarnet wants to merge 12 commits into
mainfrom
fix/image-pixel-limit-followups
Open

badGarnet wants to merge 12 commits into
mainfrom
fix/image-pixel-limit-followups

Conversation

@badGarnet

@badGarnet badGarnet commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow-ups to the image pixel limit added in #4520.

  • Charge each GIF frame at the canvas size Pillow decodes it at. A GIF frame that extends past the logical screen grows the canvas that it and every later frame are decoded onto. Pillow's frame count (n_frames) does not apply that growth, so the limit charged every frame at the first frame's size, and a small first frame followed by a large one was undercounted.
    • With hi_res, a GIF's frames are now measured by _iter_gif_canvas_sizes(), which reads each frame descriptor's position and size from the file.
    • It skips color tables and image data by their lengths, and stray bytes as Pillow does, without decompressing anything, so the work is linear in the file size.
    • It yields the running canvas size: the logical screen grown to fit every descriptor seen so far.
    • Other formats are unchanged.

Verification

  • Exact match: on 2,000 generated GIFs with random logical screens, frame positions and sizes, extensions and stray bytes, the walker's per-frame canvas sizes equal the sizes Pillow decodes each frame at. This was run in a memory-capped container.
  • Never fewer pixels: on 329 byte-mutated GIFs that Pillow could still decode, the walker never charged fewer pixels than Pillow decoded.

Tests

  • GIF walker: matches Pillow's decoded canvas on a fixed canvas, growth on a later frame, growth by offset, and a first frame past the screen (with an extension and a stray byte).
  • Growing canvas: a GIF whose canvas grows from 10×10 to 100×80 is admitted at its exact decoded total and rejected one below it, allocating no pixel buffer.
  • Frame-header failure: a failure after one frame was measured, through partition_image. No partitioner, OCR or conversion runs, the original exception escapes, and the borrowed stream stays open at its position.
  • ocr_only negative controls: whether requested directly or as hi_res's fallback when unstructured_inference is missing, ocr_only never reaches a later frame or a declared frame count, for TIFF, APNG and GIF. A hi_res control shows the same poison trips when frames are read.
  • Forged frame count: a deterministic work count for a forged APNG acTL: one run, not one per declared frame.

🤖 Generated with Claude Code

Review in cubic

badGarnet and others added 10 commits October 1, 2026 19:56
Every frame of a multi-frame image (e.g. TIFF) is decoded to RGB, and
hi_res holds all frames in memory at once, but blank frames compress to
almost nothing: a ~195 KB TIFF of 30 blank 10,000 x 10,000 frames decodes
to ~9 GB of RGB. PIL's per-frame limit does not bound the total.

Read each frame's size from its header in partition_pdf_or_image() before
any strategy runs, without decoding pixels, and raise
UnprocessableEntityError once the frames exceed IMAGE_MAX_TOTAL_PIXELS in
total (default 500,000,000, matching the per-frame limit pdf.py sets).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
PIL raises DecompressionBombError at open or seek for a single frame over
twice MAX_IMAGE_PIXELS, before the frame loop can total the pixels, so
the caller got PIL's error instead of UnprocessableEntityError. Translate
it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review follow-ups:

- ImageSequence.Iterator calls each format's seek(), which allocates the
  frame (pi-heif) or decodes it (APNG, GIF) before its size can be
  charged, so the guard could allocate a large secondary frame before
  rejecting it, and ocr_only, which uses only the first frame, now
  processed every frame. Iterate frames only for TIFF, whose seek() just
  reads the frame's IFD; for other formats charge the first frame, whose
  size Image.open() reads from the header.
- Apply the guard after the strategy is resolved: only hi_res, which
  decodes every frame, is charged for all of a TIFF's frames.
- Cover: no pixel allocation or frame load while measuring (TIFF and
  APNG), mixed frame sizes, stopping at the frame that passes the limit,
  a rejected borrowed file left open at its position, the hi_res and
  ocr_only dispatch, and PDFs bypassing the image limit.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…coding

hi_res decodes every frame of any multi-frame image, but the guard charged
only the first frame of formats other than TIFF, so many frames under the
limit could add up past it. Every frame is now charged for hi_res, each
measured without allocating or decoding it:

- TIFF and MPO frames, which can differ in size, from each frame's header
  (their seek() is metadata-only);
- HEIF images from the sizes pi-heif reads from the container on open
  (its seek() allocates the frame, so it is not used);
- canvas-composited frames (APNG, animated WebP, GIF) as canvas size times
  the frame count Pillow reads from metadata.

Covered for each format with an instrumented allocator, using a small
two-image HEIF fixture since pi-heif cannot encode.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Pillow accepts a bare int for an RGB putpixel() (setting only the red
band), but a tuple states the intent and does not rely on that leniency.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rame

The guard charged canvas-composited formats by repeating the canvas size
n_frames times, one addition per frame. Pillow takes an APNG's n_frames
straight from its acTL chunk, which may declare up to 2^31 frames, so a
few-hundred-byte 1x1 APNG cost 500 million iterations before rejection.

Measure frames as runs of same-sized frames: TIFF, MPO and HEIF yield one
run per frame physically in the file, and canvas formats one run of
n_frames, compared against the limit in constant time. The error still
names the frame that takes the total past the limit.

Also cover: auto and the dependency fallbacks resolving which frames are
charged, and a borrowed file left open at its position when reading a
frame header fails. Document that the limit bounds decoded pixels, not
allocations inside native decoders or process-wide memory.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	unstructured/__version__.py
#	unstructured/partition/utils/config.py
# Conflicts:
#	CHANGELOG.md
#	unstructured/__version__.py
#	unstructured/partition/utils/config.py
A GIF frame that extends past the logical screen grows the canvas Pillow
decodes it and every later frame onto, but Pillow's frame count does not
apply that growth, so the pixel limit charged every frame at the first
frame's size and a small first frame followed by a large one was
undercounted. Read each frame descriptor's position and size from the
file instead, skipping color tables and image data by their lengths and
stray bytes as Pillow does, without decoding anything; the running
canvas size matches the size Pillow decodes each frame at.

Also cover:
- a frame-header failure after one frame was measured, through
  partition_image: no partitioner, OCR or conversion runs, the original
  exception escapes and the borrowed stream stays open at its position;
- ocr_only, requested or as hi_res's fallback, never reaching a later
  frame or a declared frame count (TIFF, APNG, GIF), with a hi_res
  control showing the same poison trips when frames are read;
- a deterministic work count for a forged APNG frame count: one run.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-followups

# Conflicts:
#	CHANGELOG.md
#	test_unstructured/partition/pdf_image/test_image.py
#	unstructured/__version__.py
#	unstructured/partition/pdf.py

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread unstructured/partition/pdf.py Outdated
Image.open() seeks a stream to offset 0 whatever its position, so the
image it measures and partitioning later decodes start there. The frame
descriptor walker started at the caller's position instead, so a borrowed
stream positioned past 0 was parsed misaligned and its frames mismeasured.
Read from 0; the caller's position is still restored afterwards.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 2 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.

Re-trigger cubic

…-followups

# Conflicts:
#	CHANGELOG.md
#	unstructured/__version__.py

@cragwolfe cragwolfe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oracle Pro production review of exact head 2c57730165909e7d5f0634a39f13f8629106abc6 against main f4105db674e4bf6c98ddb8fe6a5e32df0d7237b4, with complete untruncated image/PDF source and tests.

One verified High-severity regression prevents approval: malformed extension parsing can admit a fixed-canvas GIF whose actual decoded frame total exceeds IMAGE_MAX_TOTAL_PIXELS. A second, nonblocking follow-up concerns scanner-handle retention when rejection tracebacks are retained. See the inline evidence and acceptance criteria.

The ordinary canvas-growth arithmetic improves main, but it does not compensate for the introduced frame-enumeration bypass. Exact-head CI reports all 53 checks successful, including the Python 3.11–3.13 unit suites. Findings are source-derived; no local tests or source changes were performed.

(authored by codex)

while (block := file.read(1)) and block != b";":
if block == b"!": # -- extension: label, then sub-blocks --
file.read(1)
skip_sub_blocks()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 / High: malformed extension can hide frames from the pixel budget. The extension label is discarded, and skip_sub_blocks() stops at the first zero. Pillow instead reads an initial extension block and then, for a non-comment extension, runs another while self.data() loop even if that initial block was empty. Insert 21 FF 00 3B, followed by 59 bytes of 41 and a final 00, between frame one and frame two of a fixed-canvas GIF. Pillow treats 3B as a 59-byte sub-block length and continues to later frames; this walker treats it as the trailer and returns normally after frame one. A three-frame 10×10 GIF with a 250-pixel budget is therefore charged 100 pixels and admitted here, whereas main's Pillow n_frames * image.size charges 300 and rejects it. The same mismatch scales past the default aggregate pixel budget while each frame remains below Pillow's individual bomb threshold.

Make extension consumption match Pillow, or reject malformed/ambiguous extensions before processing. Rejection must propagate; UnidentifiedImageError is swallowed by this checker. Add deterministic before-first-frame and between-frame fixtures, comparing real Pillow traversal against accounting; cover filename, bytes and borrowed streams, exact budget rejection, no downstream processing, and stream position/ownership. This finding is verified by source tracing, without local test execution.

(authored by codex)

if image.format == "GIF":
if isinstance(source, str):
with open(source, "rb") as f:
yield from ((1, size) for size in _iter_gif_canvas_sizes(f))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 / Medium, nonblocking follow-up: explicitly close the run generator on consumer-side rejection. For filename input this generator owns a second file handle. If the consumer raises UnprocessableEntityError after an over-budget yield, runs remains suspended inside this with open. A caller retaining the exception traceback also retains the consumer frame, its runs local, and that open descriptor. The Pillow context closes its own handle, not this scanner handle. Normal exhaustion or an exception raised inside the generator does unwind it, so this is conditional retention rather than an unconditional permanent leak.

Move owned-stream lifetime into the checker's enclosing context or explicitly close the generator on every exit. Validate closure while retaining the rejection exception/traceback, without forced GC, and retain borrowed-stream open-state/offset behavior.

(authored by codex)

This branch has not been deployed

No deployments
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.

2 participants