Skip to content

fix(arcup): accept checksum files without a trailing newline - #243

Open
osr21 wants to merge 2 commits into
circlefin:mainfrom
osr21:fix/arcup-checksum-no-trailing-newline
Open

fix(arcup): accept checksum files without a trailing newline#243
osr21 wants to merge 2 commits into
circlefin:mainfrom
osr21:fix/arcup-checksum-no-trailing-newline

Conversation

@osr21

@osr21 osr21 commented Aug 8, 2026

Copy link
Copy Markdown

What

verify_checksum_file() decided whether a checksum file was empty from the exit status of:

read -r expected_checksum expected_name < "$checksum_path"

read returns a non-zero status when it reaches EOF before encountering a newline — even though it has already assigned the variables. So a .sha256 file whose single line has no trailing newline (valid, and produced by several non-GNU checksum tools or by manual editing) was rejected with Checksum file is empty, aborting an otherwise-correct install.

Fix

Decide emptiness from the parsed hash rather than from read's exit status:

read -r expected_checksum expected_name < "$checksum_path" || true
if [[ -z "$expected_checksum" ]]; then
    error "Checksum file is empty: $checksum_path"
fi

A genuinely empty file still produces an empty $expected_checksum and is still rejected, so the empty-file guard is preserved.

Repro

printf '%s  archive.tar.gz' "$(sha256sum archive.tar.gz | cut -d' ' -f1)" > archive.tar.gz.sha256  # no trailing newline
# before: aborts with "Checksum file is empty"
# after:  verifies normally

Tests

Extends test_checksum_validation in arcup/test_arcup.sh with two cases:

  • a valid checksum file without a trailing newline now passes;
  • a genuinely empty checksum file is still rejected.

Both fail on the current arcup and pass with this change. Full bash arcup/test_arcup.sh suite is green.

Notes

Current release .sha256 assets do end in a newline, so this is a latent-robustness fix in the checksum-verification path rather than an active break — but arcup shouldn't refuse a checksum file it verifies correctly simply because of a missing trailing newline.

verify_checksum_file() decided emptiness from the exit status of
`read -r expected_checksum expected_name < "$checksum_path"`. `read`
returns non-zero when it reaches EOF before a newline, even though it has
already populated the variables. A .sha256 whose single line lacks a
trailing newline is valid, but arcup rejected it with
"Checksum file is empty", aborting a correct install.

Decide emptiness from the parsed hash instead of read's exit status.
A genuinely empty file still yields an empty hash and is still rejected.

Adds regression tests covering the no-trailing-newline and empty cases.
@wolfgang1211

Copy link
Copy Markdown

Reviewed and ran this locally. The diagnosis is right: read returns
non-zero on EOF-before-newline while still populating the variables, so the
old if ! read conflated "file has no trailing newline" with "file is
empty". Deciding emptiness from $expected_checksum is the correct fix, and
|| true is necessary rather than defensive given the script runs under
set -euo pipefail.

Verified the tests actually pin the bug: restoring the pre-fix arcup/arcup
while keeping the new cases makes the no-trailing-newline assertion fail with
Checksum file is empty (exit 1). With the fix applied, the full suite passes
all 27 assertions (exit 0), including both new cases, and shellcheck stays
clean.

I also probed the nearby edge cases manually. A newline-only file and a
spaces-only file are both rejected as empty. A mismatched hash without a
trailing newline reaches the normal downstream comparison and is rejected
with Checksum verification failed, so the untested mismatch/no-newline
matrix entry is only a coverage nit, not a behavioral gap.

For sequencing context: these regression tests do not run in CI today because
nothing invokes arcup/test_arcup.sh. #247 wires that suite into a dedicated
job; the PRs are independent, but this coverage becomes continuously enforced
once that CI wiring lands.

@osr21

osr21 commented Aug 9, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough pass — restoring the pre-fix script against the new tests to confirm they actually pin the bug is exactly the verification this kind of change needs, and I appreciate you probing the newline-only/spaces-only edges manually.

Took the coverage nit as actionable rather than leaving it a nit: pushed 01694c4, which adds the missing matrix entry — a wrong hash with no trailing newline:

# A wrong hash without a trailing newline must still fail the comparison,
# not slip through the emptiness check.
printf '%s  %s' "0000...0000" "$archive_name" > "$checksum_file"
expect_fail "mismatched checksum without trailing newline fails" verify_checksum_file ...

As you observed, it was never a behavioral gap — the value flows to the normal comparison and fails there — but it's the one cell where a future regression in the emptiness check could theoretically have flipped an invalid file to accepted, which is the dangerous direction for a checksum verifier (the original bug only broke in the safe direction, rejecting valid files). Now all four newline×validity combinations are pinned: valid+newline (pre-existing), valid+no-newline and empty (this PR's originals), mismatch+no-newline (new). Suite is green at 28 assertions locally.

Agreed on sequencing with #247 — independent merges, and the coverage here becomes continuously enforced the moment that CI wiring lands. From this side there's nothing left pending; ready for maintainer review.

@wolfgang1211

Copy link
Copy Markdown

Nice — the asymmetry argument is the better framing; the original bug only
ever failed safe, so that cell was the one worth pinning. Matrix looks
complete now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants