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>
190 lines
10 KiB
Markdown
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.
|