fix(release): read checksum manifests the way coreutils writes them - #1330
Merged
Conversation
justinjoy
force-pushed
the
fix/1311-checksum-escaping
branch
3 times, most recently
from
September 3, 2026 03:23
e1c7c98 to
8fc8c59
Compare
This was referenced Sep 3, 2026
justinjoy
force-pushed
the
fix/1315-manifest-unescape
branch
from
September 3, 2026 09:25
679bb8c to
329ce3e
Compare
verify-release.sh rejected any archive whose name coreutils escapes, before
comparing a single hash:
checksum manifest does not name back\slash.tar.gz: back\\slash.tar.gz
GNU coreutils escapes exactly three characters in a name -- backslash as `\\`,
newline as `\n`, carriage return as `\r` -- and prefixes the whole line with a
literal `\`. The set was determined by scanning all 254 legal filename bytes
through sha256sum, not from documentation: tab, other control bytes and UTF-8
are not escaped. b3sum escapes the same three with the same marker.
A third failure had nothing to do with escaping. The name was read as awk's
`$2`, and coreutils does not escape spaces, so `has space.tar.gz` was truncated
to `has` and reported as a name mismatch. The name is now taken as everything
after the two-character separator, which is also correct for binary mode.
Compare against the ESCAPED form of the expected name rather than unescaping the
manifest's. Unescaping is ambiguous in the wrong direction: a file genuinely
named `a\nb.tar.gz` is written `a\\nb.tar.gz`, and replacing `\\` then `\n` in
sequence turns it into a newline. Measured -- the naive form corrupts that name.
Escaping is one-way and has no such case, which is why `a\nb.tar.gz` is a
regression case rather than an incidental one.
Every path still fails closed. The name check only selects which manifest line
is trusted; the hash is recomputed independently, so no name-check outcome can
produce a false "verified".
This makes #1311's leading-backslash strip in hash_of reachable for the first
time and pins it, so the comment declaring it deliberately unpinned is replaced.
Refs #1315
justinjoy
force-pushed
the
fix/1315-manifest-unescape
branch
from
September 3, 2026 14:29
329ce3e to
72efd1b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1315. Stacked on #1314 (
fix/1311-checksum-escaping) — review that first.The bug
verify-release.shrejected any archive whose name coreutils escapes, before comparing asingle hash:
GNU coreutils escapes exactly three characters in a name —
\→\\, newline →\n,carriage return →
\r— and prefixes the whole line with a literal\.The escape set came from scanning all 254 legal filename bytes through
sha256sum, notfrom documentation. My first version had only two: I'd derived the set from the two cases
the issue happened to name, and missed CR. The reviewer caught it by running the scan I
should have run — an incomplete set treated as complete, which is the same defect as the bug
being fixed, reproduced inside the fix. Tab, other control bytes and UTF-8 are not escaped;
b3sumescapes the same three with the same marker.A third failure had nothing to do with escaping. The name was read as
awk '$2', andcoreutils does not escape spaces, so
has space.tar.gzwas truncated tohas. The name isnow taken as everything after the two-character separator, which is also correct for binary
mode (
hash *name).Why escaping the expected name, not unescaping the manifest's
The issue suggested unescaping. That direction is ambiguous, and I only found out by
measuring it: a file genuinely named
a\nb.tar.gzis writtena\\nb.tar.gz, and replacing\\then\nin sequence corrupts it toa<newline>b.tar.gz. Escaping is one-way andhas no such case — which is why
a\nb.tar.gzis one of the regression cases rather than anincidental one.
Fail-closed, verified
The name check only selects which manifest line is trusted; the hash is recomputed
independently. Reviewer confirmed no input produces a false "verified" — corrupt archive with
an escaped name, a forged name field carrying another file's hash, multi-line manifests with a
wrong first line, empty and hash-only manifests are all rejected. It also checked that
escaping stays injective across all three rules, so a manifest for an LF-named file is
correctly rejected against a CR-named archive.
Tests
Five cases in
scripts/ci/test-release-roundtrip.sh, built from realsha256sum/b3sumoutput — a hand-written unescaped manifest would pin the parsing without exercising the
escaping condition at all.
\rrule\nrule\\rule\nverify-release.shEach rule is independently pinned, and the ordering the comment asserts is executable
rather than prose. Verified on real bash 3.2.57 (the macOS floor) as well as 5.3, with no
temp-directory leftovers.
This also makes #1311's leading-backslash strip in
hash_ofreachable for the first time andpins it, so the comment declaring it deliberately unpinned is replaced.
Left out deliberately
make-tarball.sh's missing--/CDPATH=hardening → that is make-tarball.sh: manifest writing lacks the CDPATH/--/-P hardening applied to the reading side #1316, open andunassigned. It has no matching escaping defect: the archive name is machine-generated behind
a version regex, so nothing escapable can reach its manifests.
basenameinside a commandsubstitution — a different line and a different fix. Filed as verify-release.sh: an archive name ending in a newline is truncated before the manifest comparison #1329 rather than folded
into make-tarball.sh: manifest writing lacks the CDPATH/--/-P hardening applied to the reading side #1316, on the reviewer's argument that make-tarball.sh: manifest writing lacks the CDPATH/--/-P hardening applied to the reading side #1316 is scoped to the writing side and could be
closed while leaving this unfixed and untracked.
Local: 306 Ok / 0 Fail / 12 Skipped, serialized, dedicated build dir.