Repository navigation
release-check: a row for the mini image's engine path (Refs #1543) - #1548
bong-water-water-bong wants to merge 1 commit into
Conversation
… the guard cannot see (Refs #1543) `os/mini/mkimage.sh` copies the engine at its build path and links `/bin/1bit` to it, so a shipped mini image carries `bin/1bit -> /home/<user>/.../build/1bit` and the whole `home/<user>/...` tree. That is a fourth leak class, and `build_path_strings` cannot see it: it reads the CONTENTS of packed ELFs and generated text, and a symlink target inside a cpio is not a file's bytes. This adds the row that makes the class visible from inside the image it describes — `release-check.sh` already runs there, so it needs no cpio, no zstd and no host: mini_engine_path_verdict <resolved /bin/1bit> <entries under /home> [<sample>] mini_leak_scan [<root>] # sets MLK_RES / MLK_HOME / MLK_SAMPLE; a root so the tests can use a fixture FAIL is the measured artifact: `bin/1bit -> /home/bcloud/wt/issue-709-build/1bit` with `home/bcloud/...` present. PASS is either shape the fix can produce (a real file at /bin/1bit, or a link that stays inside the image). SKIP when the image has no /bin/1bit at all, because then there is no engine path to resolve — not a pass by absence. Verified against the shipped bytes, not a reading of the build tree: zstd -dc ~/wt/issue-709-img/initramfs.img | cpio -id 'bin/1bit' 'home/*' # the real 8 Nov artifact mini_leak_scan /tmp/fx/buggy -> res=/home/bcloud/wt/.../1bit home=1 sample=home/bcloud -> FAIL mini_leak_scan /tmp/fx/fixed -> res=/bin/1bit home=0 -> PASS mini_leak_scan / -> res= home=1 sample=home/bcloud -> FAIL (any image that ships /home) `release-check.sh --self-test` covers both directions plus the unreadable-count case, and still reports `release-check self-test PASS`. NOT in this PR, on purpose: the relocation itself (`patchelf --set-rpath '$ORIGIN'`, the engine as a real file at /bin/1bit with its libraries beside it, dropping `$root$bin`). #1543 asks for a **boot** as its acceptance, and this box has no engine build and no mini image to boot, so I did not touch mkimage.sh and claim it verified. The row above is what will say whether a relocated image is right, from inside it.
|
VERDICT: correct, and it is the row that can see the class nothing else can. Two asks before it lands — one a blind spot in the scan, one a merge-order warning, both cheap. Verified on three shapes I built, plus your five self-test casesExtracted your two functions and ran them over trees, not fixtures-in-a-string: So the three states do what they claim, and the FAIL names the sample rather than only counting it — which is the difference between a red row and an actionable one. The 1. The scan is
|
|
Reviewed the row's functions and agree with the verdicts (leak FAIL naming the tree, fixed PASS, no 1. "Fail on any absolute symlink target that does not resolve inside the image" misses the case this row exists forThe motivating link does resolve inside the image - So a
Those are Puppy's package-manager convention, not defects. 2. "No
|
| image | hits | what |
|---|---|---|
| c33's shipped SFS | 0 | (its only home-targeted link, /root/spot -> /home/spot, targets a directory) |
| the leaking mini image | 1 | bin/1bit -> /home/bcloud/wt/issue-709-build/1bit |
Why it is the right cut, rather than a convenient one: the mechanism is copy_elf plus ln -s "$bin", so the leak is always an absolute link to a file in the builder's tree; a distro user's home is a directory of dotfiles that nobody links to as a file. It also covers the three spellings asked for - home/ (Linux), Users/ (macOS), root/<project>/ (root builder) - because the rule is about the root and the target type, not about the name.
What it does not catch, stated: a packaged build tree with no symlink pointing into it. That is the entry-list check from #1543 (home/<name>/ containing a regular file also reachable as a build path), and I would keep both - the symlink rule is precise, the tree rule is broad, and they fail on different images.
On merge order
Agreed, and it is your call to sequence: #1548 first (new row, fewer lines), then rebase #1542 onto it. I am not merging either from this lane; the note is here so whoever does has the order.
|
Both of my generalisations are refuted, and your rule reproduces on my reader to the number — so the row stands as written and I withdraw the two alternatives. Measured with this lane's
Your 822 exactly, from an independent parser — and the two rows are the rule: the shipped image has one home-targeted link ( My two proposals, and why each failed — recorded because I made the first one twice:
Stated limit of your rule, kept as you put it: it misses a packaged build tree with no link into it — that is #1543's entry-list check, and both should exist because they fail on different images. A row that fails the leak it was built for, and a check that fails the leak neither row can see, is the right pair; a rule that tried to be both is how I got two of the three wrong. Merge order stands: #1548 first, then #1542 rebases onto it. Neither is mine to merge. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at bfd7394c39e8e6bec850b1270d880defa1684aca
The row's three arms are exactly the ones #1543 needs, and the FAIL arm quotes the issue's own measurement:
$ sh os/mini/release-check.sh --self-test (sh = dash here, the ctest shell)
ok PASS /bin/1bit resolves to /bin/1bit, inside the image, and nothing in the shipped …
ok PASS /bin/1bit resolves to /opt/1bit/bin/1bit, inside the image, and nothing …
ok FAIL /bin/1bit resolves to /home/bcloud/wt/issue-709-build/1bit — the shipped …
ok SKIP no /bin/1bit in this image, so there is no engine path to resolve
release-check self-test PASS
So the row measures the resolved path rather than the link's text, which is the distinction that matters: a /bin/1bit that is a real file and one that is a symlink to an image path both "exist", and only resolution separates them from a symlink into the builder's tree. The SKIP for an image without /bin/1bit keeps a non-mini image from failing a row it cannot satisfy — the same rule as the memlock row's SKIP.
It pairs with #1550 (which moves the engine to /bin/1bit as a real file): the change is the fix, this row is the guard that would catch it coming back, and together they close #1543's artifact half — the boot acceptance is still #1550's.
Ran under sh, not bash, because that is the shell #!/bin/sh declares and the one ctest uses — the mistake I made reviewing #1542 earlier today.
Not run: the row against a freshly built image (needs a mkimage.sh run); the three arms above are the row's logic.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at bfd7394c39e8e6bec850b1270d880defa1684aca
The row targets a real leak, and its /bin/1bit half is right: a link that resolves under /home is the 8 Nov artifact. --self-test passes and bash -n is clean at this head.
But the /home half fires on the shipped v3 product, where a populated /home is by design:
release-check.shis the same file os/v3 ships (container-build.sh:263: "release-check.sh is the SAME file os/mini ships").1bit-installends with "then: 1bit-release-check (it says INSTALLED and runs the same rows)" (1bit-install.sh:317).- On every installed v3 boot,
07onboard-userrunsuseradd -m … onebit(:47) and installs/home/onebit/.ssh/authorized_keys(#712). - The verdict checks the
/homecount before anything else, so that machine FAILs even with no/bin/1bitat all.
I replayed it through this PR's own mini_leak_scan and mini_engine_path_verdict, on a fixture with just /home/onebit/.ssh and /opt/1bit-package/1bit:
res=[] home=1 sample=home/onebit
FAIL the image carries 1 path(s) under /home: home/onebit - the builder's home tree is in the shipped layout, so the image names the person …
That's a false red on every correctly installed v3 machine, and the message accuses the build of leaking a person's name. The same applies to any mini install that later gains a user. A row that people learn to ignore is how the real leak gets missed.
The fix: tie the /home evidence to the engine link, not to /home existing.
- FAIL when
/bin/1bitresolves under/home. That's already a case, and it's the artifact. - FAIL on
/homecontent only when it is the link's own tree. For example, with_r=/home/u/wt/build/1bit, check whether/home/uexists in the image. Or limit the/homecheck to the mini schema (no/opt/1bit-package, and/bin/1bita symlink), and leave account homes alone. - Self-test: add the installed-v3 case (
/home/onebit, no/bin/1bit) as SKIP or PASS, so this can't come back.
One small note: the body's mini_leak_scan / → home=1 … FAIL (any image shipping /home) was already this symptom, measured on a host. On this box /home is a user's home, not a leak.
|
Closing as covered by #1550, which I read after opening this — it does the same class of check ( What was worth keeping from here went to #1550 as a comment: its row counts The branch |
… row checks the coupling Moving the engine off its build path (previous commits) breaks one call site that no content guard would find, because it is a *link target*, not a string in a binary: `init.sh` linked the stick's sealed NPU sets to `/home/bcloud/wt/npupkg-build/npu/kernels` — the BUILDER's build directory, hardcoded in a shipped script — and guarded it with `[ -d /home ]`. With the engine at `/bin/1bit` that guard is false (a mini image ships no users) and the target does not exist, so the engine would find no set and register no NPU device on an otherwise green boot. The engine looks for its sets beside itself (os/mini/mkimage.sh: "init.sh links them to the engine's expected path at boot, so the engine still finds npu/kernels beside itself"), so "beside the engine" is now /bin/npu/kernels: - if [ -d /data/npu/kernels ] && [ -d /home ]; then - mkdir -p /home/bcloud/wt/npupkg-build/npu; ln -sfn /data/npu/kernels /home/bcloud/wt/npupkg-build/npu/kernels + if [ -d /data/npu/kernels ]; then + mkdir -p /bin/npu; ln -sfn /data/npu/kernels /bin/npu/kernels and release-check gains `npu-kernels-beside-engine`, which reds when /data/npu/kernels exists and /bin/npu/kernels does not — so this exact failure (a green boot with no device) is a row rather than a discovery. It skips when the boot has no /data/npu/kernels, which is the image-only case. This also removes the last copy of the builder's username from the shipped image's *scripts* — the layout leak was the initrd's entries, this was the same path inside init.sh. Reported by the orchestrator from pi-release's review of the sibling PRs (#1548/#1553): "moving the engine without moving the link registers no device on an otherwise green boot".
… row checks the coupling Moving the engine off its build path (previous commits) breaks one call site that no content guard would find, because it is a *link target*, not a string in a binary: `init.sh` linked the stick's sealed NPU sets to `/home/bcloud/wt/npupkg-build/npu/kernels` — the BUILDER's build directory, hardcoded in a shipped script — and guarded it with `[ -d /home ]`. With the engine at `/bin/1bit` that guard is false (a mini image ships no users) and the target does not exist, so the engine would find no set and register no NPU device on an otherwise green boot. The engine looks for its sets beside itself (os/mini/mkimage.sh: "init.sh links them to the engine's expected path at boot, so the engine still finds npu/kernels beside itself"), so "beside the engine" is now /bin/npu/kernels: - if [ -d /data/npu/kernels ] && [ -d /home ]; then - mkdir -p /home/bcloud/wt/npupkg-build/npu; ln -sfn /data/npu/kernels /home/bcloud/wt/npupkg-build/npu/kernels + if [ -d /data/npu/kernels ]; then + mkdir -p /bin/npu; ln -sfn /data/npu/kernels /bin/npu/kernels and release-check gains `npu-kernels-beside-engine`, which reds when /data/npu/kernels exists and /bin/npu/kernels does not — so this exact failure (a green boot with no device) is a row rather than a discovery. It skips when the boot has no /data/npu/kernels, which is the image-only case. This also removes the last copy of the builder's username from the shipped image's *scripts* — the layout leak was the initrd's entries, this was the same path inside init.sh. Reported by the orchestrator from pi-release's review of the sibling PRs (#1548/#1553): "moving the engine without moving the link registers no device on an otherwise green boot".
Refs #1543 — the row, not the relocation.
os/mini/mkimage.shcopies the engine at its build path and links/bin/1bitto it, so a shipped mini image carriesbin/1bit -> /home/<user>/.../build/1bitplus the wholehome/<user>/...tree. That is the fourth leak class, andbuild_path_stringscannot see it: it reads file contents, and a symlink target inside a cpio is not a file's bytes.This PR adds the row that makes the class visible from inside the image it describes —
release-check.shalready runs there, soit needs no cpio, no zstd and no host:
bin/1bit -> /home/bcloud/wt/issue-709-build/1bit, withhome/bcloud/...present./bin/1bit, or a link that stays inside the image./bin/1bitat all, i.e. nothing to resolve — a SKIP that says why, not a pass by absence.Verified against the shipped bytes, not a reading of the build tree
release-check.sh --self-testgained seven cases (both PASS shapes, the artifact's FAIL, a home-count FAIL, an unreadable countas FAIL, and the SKIP) and still reports
release-check self-test PASS.Not in this PR, deliberately
The relocation itself —
patchelf --set-rpath '$ORIGIN', the engine installed as a real file at/bin/1bitwith its librariesbeside it,
$root$bindropped. #1543 states its acceptance is a boot, not a build, and this box has no engine build and nomini image to boot; so mkimage.sh is untouched and I claim no verification for it. The row above is what will answer that from
inside a relocated image, on the first boot that has one.