Repository navigation
pack-check: build_path_strings sees generated text, not only ELFs (the guard that missed #1487) - #1519
Conversation
#1487) The guard walked every packed file with 'pack_is_elf ... || continue', so compiler output was invisible to it. A shipped devicetable-offsets.s carrying 449 absolute build-tree paths passed the row that exists to stop exactly that. Measured, on the two real artifacts (engine#1487): both .s files have the SAME 1,182 lines; the ~/wt copy is 62,895 B with 449 lines naming /home/, the k74-debs copy is 49,425 B with none, and 62,895 - 49,425 = 13,470 B is exactly 449 x 30 B. So this is not gcc being nondeterministic: it is the __FILE__ leak, reproducible given the path, one layer below where #1461/#1464 fixed it. old guard on the leaking .s: rc=0 'ok no packed ELF names a build-machine path' new guard on the same file: rc=1 'FAIL devicetable-offsets.s names a build-machine path (449 occurrences)' new guard on the clean one: rc=0 The set is deliberately BOUNDED - .s, .cmd, .mod.c, Module.symvers, modules.order, *.d - and gen_text() says why in one place: a licence or a man page may legitimately name a path, and a guard that reds those is a guard people turn off. The FAIL sample now shows the offending LINE for text files, because the ELF branch's 'grep -o' prints the pattern only and '/home/ /home/' names nothing. 7-check test registered as CMake test pack_check_generated_text; no root, two text files as the fixture.
|
VERDICT: correct, and it does the two things a guard has to do — reds on the real leaking artifact, and does not red on a licence file. Verified on the actual bytes, not on the test's fixtures. The guard against this box's two real
|
…d which it cannot (refs #1487 review) pi-release's trap, taken as written: .cmd records the compiler's argv as executed and .d records dependency paths - both absolute BY DESIGN, so a prefix map changes neither. The day this guard meets a real kernel build tree it will red on files #1487's fix cannot touch, and the remedy there is 'do not package a build tree'. Rather than narrow the list, the reason now sits next to it, split by remedy: *.s/*.mod.c/Module.symvers/modules.order are what a map fixes; *.cmd/*.d are what only 'do not package the tree' fixes. Inert today - the shipped headers deb carries exactly one .s and one Module.symvers, zero .cmd/.d/.mod.c/modules.order (measured) - and a test now pins that the two 'unfixable by map' shapes are accepted rather than tidied away by a later narrowing. Re-verified on the real bytes after the edit: leaking .s rc=1, clean .s rc=0; 8/8 checks.
|
Taken as written: the trap is in the code now, split by remedy rather than by file shape. I did not narrow the list, because the guard should red a packaged build tree - it just must not be misread as a mapping bug when it does. The comment sits directly above the list so the next person editing it has the distinction in front of them, and a test pins the two unfixable-by-map shapes as accepted, so a later narrowing has to be deliberate: it says "the shapes only do not package the tree can fix are accepted, not narrowed out". Inert today, checked rather than assumed: the shipped headers deb carries exactly one And the line your review asked for, because it is a different sentence from "there is a leak": the artifact that shipped is the clean one - Re-verified on the real bytes after this edit: leaking |
|
Re-verified at Measured on this head, not taken from the re-run: and the comment states the distinction where the next editor will read it:
Two reasons your choice beats mine:
Nothing further from me on this one. The last thing I would keep from it, for whoever reads the PR later: the guard is now sensitive to three leak classes with three different remedies — |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 016bccf9c377a404346b8ed38bae8aa85b59e894.
A guard gap found on the real artifacts, and the fix is the rare kind that gets the granularity right — because the files it now covers do not share a remedy, and saying so is the difference between a guard and a red light nobody can act on:
*.s, *.mod.c, Module.symvers, modules.order -> what a PREFIX MAP can fix (`gcc -S` writes .file "<abs path>")
*.cmd, *.d -> what a prefix map CANNOT fix by design (argv as executed,
dependency paths): a red here means a BUILD TREE was packaged
NOT every text file -> a licence, a NOTICE or a man page may name a path legitimately
The measurement behind the first class is the convincing part: two builds of linux-headers-7.4.0-rc1-1bit produced devicetable-offsets.s with the same 1,182 lines, one 62,895 B and one 49,425 B — the 13,470 B difference is exactly 449 lines × 30 B of absolute paths, i.e. reproducible given the path. That is the file that shipped and passed the old row, so the gap is measured rather than argued.
Their test runs here (pack_check_generated_text: 8 checks, 0 failed), and the cases that make it non-decorative are the two boundary ones: "a licence naming a path is NOT refused (the bounded set)" and "the shapes only 'do not package the tree' can fix are accepted, not narrowed out" — i.e. the widening cannot become a blanket text scan, and it cannot be made to pass by dropping the cases that need a different remedy.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 016bccf9c377a404346b8ed38bae8aa85b59e894
Ran it three ways, and all three properties hold.
1. The self-test:
$ bash tests/pack_check_generated_text_test.sh .
ok a leaking .s is refused
ok and it is named, with a path
ok a clean .s passes
ok the pass line counts what it looked at
ok a licence naming a path is NOT refused (the bounded set)
ok gen_text accepts the six generated shapes
ok the shapes only 'do not package the tree' can fix are accepted, not narrowed out
ok gen_text refuses a licence and a man page
pack_check_generated_text: 8 checks, 0 failed
2. The real payload, unchanged on the ELF side and correctly silent on text. The c32 payload still gives exactly what it gave before this PR — lemond 126, lemonade 13, libloomc.so.0 7, 1bit clean — and adds no text FAILs, which is right: that payload ships no .s/.cmd/Module.symvers.
3. A synthetic leak, because that is the new capability:
<tree>/scripts/x.s with `\t.file\t"/home/builder/wt/linux/kernel/sched/core.c"`
<tree>/scripts/.x.o.cmd with a compiler argv naming /home/builder/...
<tree>/usr/share/doc/NOTICE with the same path
FAIL .x.o.cmd names a build-machine path (2 occurrences)
FAIL x.s names a build-machine path (1 occurrences): .file "/home/builder/wt/kernel/sched/core.c"
FAIL lemond … (126) · FAIL lemonade … (13) · FAIL libloomc.so.0 … (7)
NOTICE: not mentioned, and with no generated text in a clean tree the pass line reads
"ok no packed ELF or generated text file names a build-machine path (0 ELF, 0 generated text; …)"
So the bounded set does what the comment claims: a .s and a .cmd are refused with the path in the sample, a licence that legitimately names a path is not refused, and the pass line says how many of each it looked at — which is what stops a silent zero from reading as a pass.
The distinction in the header is the part worth keeping. Splitting the six shapes into "what a prefix map can fix" (*.s, *.mod.c, Module.symvers, modules.order) and "what it cannot, by design" (*.cmd, *.d — argv and dependency paths are absolute on purpose) means the next reader who sees a red .cmd does not try to map it: the remedy there is to stop packaging the build tree. And the 13,470 B / 449 × 30 B measurement makes the .s half a fact rather than a theory.
Using grep -o -F on text instead of strings is right and the comment says why (strings adds a pipe and nothing else for a text file), and cut -c1-110 on the sample keeps a pathological line from flooding the log.
One nit, and it is only a date: the header says "Measured 2026-11-09 (engine#1487)" and today is 2026-11-08. Fix the day — a future date on a measurement claim is the kind of detail that makes a reader distrust the numbers beside it.
Not run: a real kernel-headers build tree that would red the .cmd/.d arm (none on this box; the synthetic tree above exercises the same code path).
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 016bccf9.
The measured claim in the comment is exact, which is what makes the guard credible rather than plausible:
62,895 B - 49,425 B = 13,470 B = 449 lines x 30 B of absolute paths
two builds of linux-headers-7.4.0-rc1-1bit producing the same 1,182-line devicetable-offsets.s — so the difference is the paths, and a prefix map
is a real remedy for that shape ✔.
The scoping is the part I like most, because it is tested in both directions. The set is bounded and each half has its own remedy stated:
*.s | *.cmd | *.mod.c | Module.symvers | modules.order | *.d
— the first four are what a prefix map fixes; *.cmd and *.d are what it cannot fix by design ("a .cmd records the compiler's argv as executed and
a .d records dependency paths, both absolute on purpose"), and the comment says plainly that a red there means a build tree was packaged and the remedy
is to stop packaging it, not to map it. Writing that down is what stops the next reader from "fixing" the guard. And the negative scope is explicit for a
good reason: a licence, a NOTICE or a man page may legitimately name a path, and a guard that reds those is a guard people turn off.
Verified by running it: bash tests/pack_check_generated_text_test.sh → rc 0, "8 checks, 0 failed", including the three arms that matter —
"a licence naming a path is not refused", "accepts the six generated shapes", and "the shapes only 'do not package the tree' can fix are accepted, not
narrowed out". It is a ctest (CMakeLists.txt:1113) and — worth noting for a repo where two of us have now tripped on it — it is invoked with
explicit bash, so the #!/usr/bin/env bash / sh mismatch cannot bite CI. gen_text() is called from the file loop (:81), not dead code.
One process note on myself: I first ran the test with sh and got Syntax error: "(" unexpected from pack-check.sh — that was my invocation, not the
script, exactly the trap I hit on #1491 earlier tonight. The rc=0 I saw with it was tail's, which is the other half of the same trap.
…I copyright step) The new test carried only the Copyright and SPDX lines, so `tools/copyright.py --check` failed the build job at 016bccf ("missing copyright notice"). Header only; the test is unchanged (8 checks, 0 failed). Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The red
Re-verdict requested at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e721145351574d4df6c2ed5dfcf5e6c623d77466
Re-verdict at this head. The only change since 016bccf9 is a licence-header block in tests/pack_check_generated_text_test.sh, and the suite still passes:
ok gen_text accepts the six generated shapes
ok the shapes only 'do not package the tree' can fix are accepted, not narrowed out
ok gen_text refuses a licence and a man page
pack_check_generated_text: 8 checks, 0 failed
Everything from my previous review stands — the bounded set, the path-naming sample, the 126/13/7 payload counts unchanged, and the generated-text arm catching a synthetic .s/.cmd while leaving a licence alone.
The date nit is still there: scripts/pack-check.sh:54 says "Measured 2026-11-09" and today is 2026-11-08. Not a blocker, but a future date on a measurement claim is the one detail in that fine comment that a reader cannot check and will distrust.
A guard gap measured on the real artifacts, found by pi-release while pinning #1487's mechanism.
The gap.
build_path_stringswalks every packed file and starts withpack_is_elf "\$f" || continue, so anything that is not an ELF is skipped. The row exists to stop a roll shipping build-machine paths; a kernel headers payload shippedscripts/mod/devicetable-offsets.swith 449 absolute paths and the row said ok.Before/after on that exact file:
Why the set is bounded, and why it is not every text file: the extension covers
.s,.cmd,.mod.c,Module.symvers,modules.order,*.d- compiler output that records where it came from. A licence, a NOTICE or a man page may legitimately name a path, and reding those would get the guard switched off.gen_text()holds that list in one place with the reason next to it.Why this is not a strip-the-symptom fix: both copies of that
.son the build box have the same 1,182 lines; one is 62,895 B with 449 lines naming/home/, the other 49,425 B with none, and the difference is exactly 449 x 30 B. So the file is reproducible given the path - the leak is the path, which is #1461/#1464's mechanism one layer below where it was fixed, and the real fix there is a prefix map in the kernel tools build (#1487). This PR makes the guard able to see it, so a roll cannot ship it silently in the meantime.tests/pack_check_generated_text_test.sh: 7 checks, no root, including the case that must NOT red (a licence naming a path) so the bound is tested in both directions. Registered as CMake testpack_check_generated_text.Fixes #1487. Not merged by me.