llm-ingestion-okf/docs/2026-09-18-bildestien-holder-0-10-1.md
Kjell Tore Guttormsen 0c3c4904ee
fix(assets): bound every link of the filter chain, and cover the backstop
The two findings of the 18.09 PM checkpoint of `0f308c1`. Red tests landed
first in `3b587ea`; this is what turns them green.

BLOCKER -- `_check_inflated` read `filters[0]`, measured that one link and
returned, which is not a bound: a PDF decodes a stream through a LIST of
filters. Measured in paired subprocesses from two pinned trees, idle machine:

  [/FlateDecode]        400 MB  408 516 B   59 232 256 ->    62 017 536 B
  [/FlateDecode x2]     400 MB    1 636 B  886 554 624 ->    52 367 360 B
  [/FlateDecode x3]     400 MB    1 070 B  889 393 152 ->    61 390 848 B
  [/FlateDecode x2]     1,2 GB    2 927 B  2 567 204 864 ->  60 403 712 B

542 000x the file at two links, and the picture WAS refused at the end -- by
`check_payload` after `get_data()`, once the memory was spent. The single-link
row is the control and does not move. It also left the 16 corpus objects behind
an `[/ASCII85Decode /FlateDecode]` chain unmeasured, since `filters[0]` is not
`FlateDecode` there.

`_check_stream_cost` walks every link. THREE CLASSES and no fourth
(`extract.bounded_pdf_filters`, pinned by a test): `FlateDecode` MEASURED, a
link with another expanding link behind it inflated under the same bound and
handed on; `ASCII85Decode`/`ASCIIHexDecode` bounded by their own input because
they SHRINK; `DCTDecode`/`JPXDecode`/`JBIG2Decode` PASS THROUGH. Everything
else -- `LZWDecode`, `RunLengthDecode`, `CCITTFaxDecode`, `/Crypt`, anything
written later -- is refused UNREAD with a new code `asset_pdf_unbounded`,
decided before the FIRST link is decoded so a document cannot make the run pay
for the links in front of the one we cannot bound. An encrypted stream is
deciphered and then measured, where `stream.decipher is not None` used to
return unmeasured; 0 of 5 142 objects here are in an encrypted document, which
is why nothing caught it.

NOT ONE PICTURE CHANGES HANDS, AND IT IS MEASURED BY NAME. Every PDF on this
machine -- 78 documents, K2 in both trinn1 and trinn2, the shipped fixtures and
R761 -- run through `_pdf_images` page by page from both pinned trees:

  images carried          9 356 -> 9 356
  documents losing one              0 of 78
  documents gaining one             0 of 78
  asset_pdf_unsupported     322 -> 314
  asset_pdf_unbounded         0 -> 8

The 8 are the 4 `CCITTFaxDecode` stencil masks (`/ImageMask true`,
`/BitsPerComponent 1`), counted twice because trinn1 and trinn2 hold the same
document. They were refused before and are refused now, one step earlier and
under a code that says why.

MAJOR -- `check_payload(len(data))` after `get_data()` is the counted refusal
four documentation surfaces point at, and deleting exactly that line passed all
2 132 tests. It is reachable through a stream pdfminer has ALREADY decoded
(`decode()` sets `rawdata` to `None`), which is now the ONLY case outside the
bound and has a test.

Eight mutations, one line each, every one DEAD, with the unmutated tree run
first as the control: first-link-only, loop dropped, inequality reversed,
encrypted skipped, backstop deleted, unknown filter passed through,
intermediate link not carried forward, whole check removed.

`tools/okf_accounting_gate.py` gains one line, the new code in
`REJECTION_CODES` -- what a rejection code requires and nothing more. Gate
unchanged: exit 1, GATE RED rows 2, 3, 6. Version stays 0.10.1, untagged.

Suite after `git add` against a clean tree: `uv run pytest -q` ->
2152 passed, 1 skipped (226 s). ruff, ruff format --check, mypy --strict clean.

Report: docs/2026-09-18-filterkjeden-og-backstoppen.md

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-18 15:34:07 +02:00

190 lines
10 KiB
Markdown

# The image path, second review: the bound bound a claim, not a cost
A second independent review, one day after the first, read `230d1cb` — the
commit that closed the two MAJOR findings of the `v0.10.0` review — and found
that one of the two was **not closed**. This is what that review found, what it
measured, and what this round changed. It is the round that makes `v0.10.1`
true; the round it corrects is
[`docs/2026-09-17-bildestien-0-10-1.md`](2026-09-17-bildestien-0-10-1.md), which
now carries a correction block where it claimed this defence.
## What was wrong: a declared size and a decompressed stream are two numbers
`check_size` read `/Width` and `/Height` out of a PDF image dictionary and
refused anything over `MAX_IMAGE_PIXELS`. Both numbers are written by the
document. `/Length` beside them is the **compressed** length, and nothing in
the dictionary states what `stream.get_data()` will return. A document that
declares `1 x 1` and hangs 400 MB of deflated zeros off it therefore passed the
check, paid the whole cost, and was **carried** as a one-pixel picture.
Measured on `230d1cb`, and re-measured here on `ed8d9d7` before anything
changed, with the same fixture in its own interpreter:
| stream inflates to | file size | declared | carried | rejection | peak RSS |
|---|---:|---|---|---|---:|
| 400 MB | 408 516 B | 1 x 1 | yes | none | **891 904 000 B** |
| 1,2 GB | 1 223 843 B | 1 x 1 | yes | none | **2 436 MB** (review) |
About 2 100x the file size, linear, so a 10 MB document is roughly 21 GB. It is
the failure mode the first review named — one document killing a batch build —
reached through the other number.
The first round's four mutations were all killed by its tests, and that told us
nothing about this: **not one of them separated a declared size from an actual
one**. A test built on an honest `20000 x 20000` declaration is green whether or
not the stream is bounded.
## What changed: three numbers are bounded, and the limit is stated
> **CORRECTION, 2026-09-18 (same day, later) — point 3 below bounded ONE LINK
> of a filter chain, not the chain.** A PM checkpoint of `0f308c1` measured
> `/Filter [/FlateDecode /FlateDecode]`: 1 636 bytes of file, 889 573 376
> bytes of peak RSS, still refused at the end by the backstop after the memory
> was spent. The sentence below that the measurement "runs before
> `get_data()`" is true; the sentence in the section after it, that the
> measurement covers the case where "`FlateDecode` is the first filter", was a
> bound on the first link and was therefore not a bound. The round that closes
> it, with the three classes of filter and the refusal for the ones no chunked
> measurement can reach, is
> [`docs/2026-09-18-filterkjeden-og-backstoppen.md`](2026-09-18-filterkjeden-og-backstoppen.md).
> That round also found the backstop this paragraph leans on had **no test at
> all**: deleting it passed all 2 132 tests.
1. **What the container DECLARES** — unchanged from the first round.
2. **What a carried FILE measures** — new. `read_image` now checks the size it
sniffs out of the header. This package never decodes such a file, so it pays
nothing for it; but a 7 000 x 7 000 PNG of 47 705 bytes written into a bundle
hands the consumer the same bomb with `7000x7000 px` printed beside it, and
the README's first sentence about this bound said such an image was refused.
Over the 4 828 image objects of the reference corpus the largest is 18.6 MP,
so nothing measured is refused by this.
3. **What a PDF image's STREAM decompresses to** — new, and the finding.
`assets.inflated_size` inflates the raw stream a chunk at a time, keeps only
a running total, and **discards the output**, so the measurement cannot cost
what the bomb costs. It runs before `get_data()`.
**The limit is stated, not implied.** The stream measurement runs where
`FlateDecode` is the first filter and the document is not encrypted (the raw
bytes are ciphertext until pdfminer deciphers them). Every other filter chain is
caught by `check_payload(len(data))` **after** `get_data()` — a counted refusal,
not a bounded one: the memory is spent and then the picture is dropped. That
distinction is the whole substance of this round, so it is written in
`_check_inflated`'s docstring rather than left for the next reviewer to find.
A legitimate image is therefore inflated twice, once to measure and once to
carry. That is the price of not holding an unbounded buffer in order to find out
how big it is, **and at this sample it does not show**: three PDFs of the K2
reference corpus, 800 carried images between them, extracted twice in one
process with the measurement on and off.
| document | images | rejections | with the bound | without |
|---|---:|---:|---:|---:|
| Bilag 3.1 Miljøteknisk rapport | 464 | 0 | 18.21 s | 18.51 s |
| Bilag 1 Kravspesifikasjon | 311 | 12 | 16.28 s | 16.88 s |
| Bilag 3.3.1 Brannkonsept | 25 | 0 | 2.70 s | 2.35 s |
The bounded arm is faster on two of the three and slower on the third, which is
run-to-run noise rather than a speedup: the honest reading is that a second
inflate of these streams is **below the noise floor** of a PDF extraction that
is dominated by text layout. What is NOT noise is that the fate of every image
is identical on both arms — 464/464, 311/311 with the same 12 rejections, and
25/25 — so the bound refuses nothing these documents carry.
### A declared size that is not a size
`/Width -1 /Height 40000000000` multiplies to a **negative** pixel count, under
which every `>` bound reads as satisfied. `check_size` returned silently, 400 MB
was decompressed, and the refusal arrived from `encode_png` as
`asset_samples_invalid` — a code about a sample buffer, for a defect in the
declaration. A non-positive dimension is now **`asset_size_invalid`**, raised
before the stream is read.
It gets its own code rather than joining `asset_too_large` because the two say
different things about a document: a publisher shipping a picture larger than
this package carries is not a dictionary written to be read wrong, and counting
them together would make a corpus statistic about the first untrue. `None` stays
UNKNOWN — a container that declares no size leaves nothing to bound, and
inventing a number would refuse a legitimate picture.
### The line that says what is missing
Two smaller findings in the same line, both introduced by the first round's fix:
- **The address was written twice**, once in a code span and once bare, and a
GFM/linkify renderer autolinks a bare URL into `<a href>`. It takes a click
rather than a render, so it is weaker than `![...](...)` — but "inert" was
half true, and half is what that line exists not to be. Written once now.
- **The caption was dropped.** `label` stayed in the signature and no branch
read it, so the alt text or figure caption of an image the bundle does not
carry was lost — a regression against 0.10.0 and against the line's own stated
reason for existing (a reader cannot weigh an absence they were never shown).
It is written again, in the same `-- <label>` form a carried pointer uses.
## Measured after
Same fixture, same machine, same command, in its own interpreter:
| stream inflates to | file size | before | after |
|---|---:|---:|---|
| 400 MB | 408 516 B | carried, 891 904 000 B peak | **refused `asset_too_large`, 57 065 472 B peak** |
| 1,2 GB | 1 223 843 B | carried, 2 436 MB peak | **refused `asset_too_large`, 64 569 344 B peak** |
The bounded figure barely moves when the stream triples, because what grows is
the compressed input, which was already in memory. **The cost no longer scales
with the bomb**, which is the property the first round did not have.
End to end through the shipped CLI, default flags, the bomb beside one ordinary
markdown document so the build has a plan to write:
```
$ /usr/bin/time -l okf build /tmp/okf-r3/in --bundle /tmp/okf-r3/bb \
--bundle-id t3 --okf-version 0.2
exit=0
79650816 maximum resident set size
$ ls /tmp/okf-r3/bb/assets
ls: .../assets: No such file or directory
$ grep Images /tmp/okf-r3/bb/log.md
* **Images**: 0 carried of 1 found, ...
```
The review measured 838 000 640 B and an `assets/…-im0.png` written after it.
The concept says what stood there:
```
Image: page-1-Im0 (not carried: the stream behind 'page-1-Im0' decompresses to
more than 268435456 bytes from 407685 bytes of input, over this package's
bound; refused without being held, because the size a container declares is a
claim and this is the cost)
```
## The version
`v0.10.1` was going to be cut from a tree that said `0.10.0` in nine places: two
bound to each other by a test, and seven bound to nothing — four README install
lines telling a consumer to install the release before this one, two prose lines
about what that tag declares, the "current tag" entry, and a CHANGELOG whose
0.10.1 content sat under `[Unreleased]`.
Two new tests in `tests/test_packaging.py` bind all of them to
`llm_ingestion_okf.__version__`, plus the guard tag in the README to the one in
`[tool.uv.sources]`, which is the same class of instruction going stale. Proven
red in the intended way: bumping `__version__` and `pyproject.toml` alone turned
the first one red with `install lines name ['v0.10.0']`.
## Not measured
- **The 43-document reference corpus was not rebuilt.** The `render_missing`
change adds `-- <label>` to the line for an image a bundle does not carry, so
an assets-on corpus build differs on exactly those lines. That is the repair,
not a side effect, but its size across that corpus is unmeasured here. What
IS measured: **0 shipped artifacts move**. No bundle under `examples/`,
`skills/` or `tests/fixtures/` carries an image pointer of either kind — 0
occurrences of `Image: ` against a known-positive control on the same
directories — so both this change and the new `read_image` bound move zero
committed bytes.
- The pinned `K2-bundle-default-20260912` predates the image path entirely and
cannot move.
- The review's own "not measured" list stands: the content accounting after this
change, R761's `50 carried of 50`, docx/pptx/xlsx zip-slip, SVG as active
content, and the `[ocr]` path.