fix(assets): bound what the run pays, not what the document claims (0.10.1)
A second independent review read `230d1cb` -- the commit that closed the `v0.10.0` review's two MAJOR findings -- and found one of them open. The bound read `/Width` and `/Height`, which an untrusted document writes, while `get_data()` pays for the stream beside them; `/Length` is the COMPRESSED length and the two numbers are independent. Re-measured here on `ed8d9d7` before anything changed, in its own interpreter: a 408 516-byte PDF declaring 1x1 and carrying 400 MB of deflated zeros was CARRIED, no rejection, 891 904 000 B peak RSS. After: 0 carried, `asset_too_large`, 57 065 472 B. At 1,2 GB of zeros, 2 436 MB -> 64 569 344 B -- the cost no longer scales with the bomb. End to end through the CLI with the shipped defaults: 838 000 640 B and an asset written -> exit 0, 79 650 816 B, `0 carried of 1 found`, no `assets/`. Three numbers are bounded now, not one: what a container DECLARES, what a carried FILE measures (`read_image`, so a 49 MP PNG of 47 705 bytes is not passed on to a consumer), and what a PDF stream DECOMPRESSES to (`assets.inflated_size`, a chunk at a time, output discarded, before `get_data()`). The limit is stated rather than implied: the stream measurement runs where `FlateDecode` is the first filter and the document is not encrypted; every other chain is a check on the decoded length AFTER the decode, a counted refusal and not a bounded one. A non-positive declared dimension is `asset_size_invalid`, its own code, raised before the stream is read. `-1 x 40000000000` is a NEGATIVE pixel count, under which every `>` bound read as satisfied, so the check returned silently and the refusal arrived from `encode_png` as `asset_samples_invalid`. Its own code because a publisher shipping a picture bigger than this package carries and a dictionary written to be read wrong are different facts about a document. Two smaller findings in the line that says what is missing, both introduced by the first fix: the address was written twice, once bare, and a linkifying renderer autolinks a bare URL -- written once now, in one code span; and `label` became a dead parameter, so the figure's caption was dropped, a regression against 0.10.0. It is written again in the `-- <label>` form a carried pointer uses. Version bumped to 0.10.1 across all ten places. Nine were unbound and stale: four README install lines naming the previous release, two prose lines, the "current tag" entry, `uv.lock`, and a CHANGELOG whose 0.10.1 content sat under `[Unreleased]`. Two new packaging tests bind them to `__version__`, and the README's guard tag to `[tool.uv.sources]`. Every test was red first. The fate of every image is identical with and without the new bound on three K2 PDFs carrying 800 images (464/464, 311/311 with the same 12 rejections, 25/25), and the second inflate is below the noise floor there. 0 shipped artifacts move: no bundle under `examples/`, `skills/` or `tests/fixtures/` carries an image pointer at all, measured against a known-positive control. `asset_too_large` was undocumented in the error registry; both codes are there now. `tools/okf_accounting_gate.py` gains the new code in its closed list -- one string, no behaviour change, stated because that file belongs to another order. Suite 2141 passed / 1 skipped, ruff + format + mypy --strict clean. Report: docs/2026-09-18-bildestien-holder-0-10-1.md Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
ed8d9d709f
commit
0f308c1f56
15 changed files with 842 additions and 81 deletions
|
|
@ -74,12 +74,28 @@ Decoding first gives `asset_pdf_unsupported`; reading the size first gives
|
|||
`asset_too_large`. `encode_png` refuses the same size on its own, so the
|
||||
encoder does not trust its caller.
|
||||
|
||||
> **CORRECTION, 2026-09-18 — this paragraph claimed a defence this round did
|
||||
> not build.** A declared size and a decompressed stream size are two
|
||||
> independent numbers: `/Length` is the COMPRESSED length, and nothing in the
|
||||
> dictionary states what `get_data()` returns. A second independent review
|
||||
> measured a 408 516-byte PDF declaring **1x1** and carrying 400 MB of
|
||||
> deflated zeros being CARRIED, with no rejection, at **892 MB of peak RSS** —
|
||||
> the same failure mode this round set out to close, reached through the other
|
||||
> number. The bound was real and it bound the wrong thing. What this round
|
||||
> DID close is the declared size and the `data:` URI; the stream itself is
|
||||
> bounded from `docs/2026-09-18-bildestien-holder-0-10-1.md`, and the
|
||||
> paragraph below about a verbatim file is corrected there too.
|
||||
|
||||
**The `data:` URI, which the review flagged and did not measure,** is closed by
|
||||
the same bound, checked on the payload length before decoding (base64 expands
|
||||
by 3/4). Measured: refused with `asset_too_large` as a row.
|
||||
|
||||
Not bounded, and stated rather than hidden: an image FILE carried verbatim is
|
||||
never decoded, so its cost is its own file size.
|
||||
never decoded, so its cost is its own file size. (Corrected 2026-09-18: it is
|
||||
bounded now. This package pays nothing for such a file, 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 own first sentence
|
||||
about this bound said it was refused.)
|
||||
|
||||
## The determinism defect PM added to this order
|
||||
|
||||
|
|
|
|||
175
docs/2026-09-18-bildestien-holder-0-10-1.md
Normal file
175
docs/2026-09-18-bildestien-holder-0-10-1.md
Normal file
|
|
@ -0,0 +1,175 @@
|
|||
# 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
|
||||
|
||||
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.
|
||||
Loading…
Add table
Add a link
Reference in a new issue