Skip to content

Commit 2822f2d

Browse files
committed
backup: PR810 r6 — codex r5 P2 (empty CHECKSUMS + missing-target classification)
Two codex P2 findings on r5 (commit 7a8e2f3): 1. (codex P2, checksums.go:256) VerifyChecksums returned nil on a CHECKSUMS file with zero parsed rows (empty file, blank lines only). Phase 0a dumps are non-empty by construction (WriteChecksums lists every regular file), so a zero-row CHECKSUMS is always a producer-side corruption signal, never a benign shape. The verifier was hiding partial-write or truncation failures behind a "no rows checked == OK" answer. Fix: VerifyChecksums now counts verified rows and returns ErrChecksumsEmpty when the count is zero. Blank lines still skip silently (matching sha256sum -c's tolerance of trailing newlines) but do not satisfy the at-least-one-row guard. Test: TestVerifyChecksums_RejectsEmptyChecksumsFile covers three corruption shapes (empty file, blank lines only, trailing-newline only). 2. (codex P2, checksums.go:306) refuseSymlinkComponents returned raw fs.ErrNotExist when a CHECKSUMS-listed file was missing from disk. Callers branching on errors.Is(err, ErrChecksumMismatch) missed this case — partial restores and tamper-deletes produced an unclassified path error instead. Fix: detect fs.ErrNotExist at the refuseSymlinkComponents call site (and at sha256File's call site as belt-and-braces) and promote to ErrChecksumMismatch. The design's contract ("Files referenced by CHECKSUMS but missing on disk surface as the same error class so a partial-restore is obvious") is now honored. Test: TestVerifyChecksums_MissingTargetIsMismatch — writes a CHECKSUMS row, deletes the listed file, asserts ErrChecksumMismatch via errors.Is. API shape: verifyOneChecksumLine returns (bool, error) so the parent loop can count "real" verified rows separately from blank-line skips. Internal helper; signature change does not escape the package. Caller audit: - verifyOneChecksumLine is internal to checksums.go; the signature change is contained. - VerifyChecksums has no production callers yet (test-only). The new ErrChecksumsEmpty and the promoted ErrChecksumMismatch shapes are additive on the typed-sentinel surface; honest CHECKSUMS still verify cleanly. Self-review: 1. Data loss — none; both fixes catch silent-corruption signals. 2. Concurrency — none. 3. Performance — one int counter per file; sha256 streaming unchanged. 4. Data consistency — empty-CHECKSUMS hole closed; missing- target now classified. 5. Test coverage — two new tests; existing thirteen CHECKSUMS tests still pass.
1 parent 7a8e2f3 commit 2822f2d

2 files changed

Lines changed: 117 additions & 9 deletions

File tree

internal/backup/checksums.go

Lines changed: 60 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/hex"
77
"fmt"
88
"io"
9+
"io/fs"
910
"os"
1011
"path/filepath"
1112
"sort"
@@ -233,48 +234,98 @@ func VerifyChecksums(root string) error {
233234
sc := bufio.NewScanner(f)
234235
sc.Buffer(make([]byte, 0, maxChecksumLineLen), maxChecksumLineLen)
235236
lineNum := 0
237+
verified := 0
236238
for sc.Scan() {
237239
lineNum++
238-
if err := verifyOneChecksumLine(root, sc.Text(), lineNum); err != nil {
240+
got, err := verifyOneChecksumLine(root, sc.Text(), lineNum)
241+
if err != nil {
239242
return err
240243
}
244+
if got {
245+
verified++
246+
}
241247
}
242248
if err := sc.Err(); err != nil {
243249
return errors.WithStack(err)
244250
}
251+
// Refuse a CHECKSUMS that yielded zero verified rows. An empty
252+
// (or all-blank) file would otherwise pass verification and
253+
// hide a producer-side corruption — the dump tree itself is
254+
// non-empty (WriteChecksums lists every regular file), so a
255+
// CHECKSUMS with no rows is always a signal, never a benign
256+
// shape. Codex r5 P2 on PR #810.
257+
if verified == 0 {
258+
return errors.WithStack(ErrChecksumsEmpty)
259+
}
245260
return nil
246261
}
247262

263+
// ErrChecksumsEmpty is returned by VerifyChecksums when the
264+
// CHECKSUMS file parsed to zero checksum rows (empty file, all
265+
// blank lines, or comment-only — none of which are valid Phase 0a
266+
// dump shapes). Typed so callers can distinguish "CHECKSUMS file
267+
// did not list anything" from "a listed file mismatched".
268+
var ErrChecksumsEmpty = errors.New("backup: CHECKSUMS contains no checksum rows")
269+
248270
// verifyOneChecksumLine handles a single CHECKSUMS line: parse,
249271
// path-validate, recompute, compare. Extracted from VerifyChecksums
250272
// so the parent stays under the project's cyclop ceiling and the
251273
// per-line failure modes (malformed shape, traversal, symlink
252274
// escape, missing file, mismatch) each surface as a distinct
253275
// typed error.
254-
func verifyOneChecksumLine(root, line string, lineNum int) error {
276+
//
277+
// Returns (verified, err): verified=true when the line was a real
278+
// checksum row that VerifyChecksums should count toward the
279+
// "at least one row" empty-file guard (blank lines do not count).
280+
// A missing file referenced by an honest CHECKSUMS row is
281+
// surfaced as ErrChecksumMismatch — partial restores and
282+
// adversarial deletions land in the same typed failure class
283+
// callers branch on. Codex r5 P2 on PR #810.
284+
func verifyOneChecksumLine(root, line string, lineNum int) (bool, error) {
255285
if line == "" {
256-
return nil
286+
return false, nil
257287
}
258288
want, rel, ok := splitChecksumLine(line)
259289
if !ok {
260-
return errors.Wrapf(ErrChecksumsMalformedLine, "line %d: %q", lineNum, line)
290+
return false, errors.Wrapf(ErrChecksumsMalformedLine, "line %d: %q", lineNum, line)
261291
}
262292
cleaned, err := validateChecksumRelPath(rel)
263293
if err != nil {
264-
return errors.Wrapf(err, "line %d", lineNum)
294+
return false, errors.Wrapf(err, "line %d", lineNum)
265295
}
266296
joined := filepath.Join(root, cleaned)
267297
if err := refuseSymlinkComponents(root, cleaned); err != nil {
268-
return errors.Wrapf(err, "line %d", lineNum)
298+
// Missing file along the resolved path is a partial-
299+
// restore / tamper signal — same operational category
300+
// as a hash mismatch. The Lstat-walk caller bubbles up
301+
// raw fs.ErrNotExist; promote it to the typed
302+
// ErrChecksumMismatch class so callers branching on
303+
// errors.Is keep covering this case. Codex r5 P2 on
304+
// PR #810.
305+
if errors.Is(err, fs.ErrNotExist) {
306+
return false, errors.Wrapf(ErrChecksumMismatch,
307+
"%s: missing on disk", cleaned)
308+
}
309+
return false, errors.Wrapf(err, "line %d", lineNum)
269310
}
270311
got, err := sha256File(joined)
271312
if err != nil {
272-
return errors.Wrapf(err, "verifying %s", cleaned)
313+
// A missing target is a partial-restore or tamper
314+
// signal; either way it is operationally identical to
315+
// a checksum mismatch for the caller. Surface as
316+
// ErrChecksumMismatch so `errors.Is(err,
317+
// ErrChecksumMismatch)` covers both cases. Codex r5 P2
318+
// on PR #810.
319+
if errors.Is(err, fs.ErrNotExist) {
320+
return false, errors.Wrapf(ErrChecksumMismatch,
321+
"%s: missing on disk", cleaned)
322+
}
323+
return false, errors.Wrapf(err, "verifying %s", cleaned)
273324
}
274325
if got != want {
275-
return errors.Wrapf(ErrChecksumMismatch, "%s: have %s want %s", cleaned, got, want)
326+
return false, errors.Wrapf(ErrChecksumMismatch, "%s: have %s want %s", cleaned, got, want)
276327
}
277-
return nil
328+
return true, nil
278329
}
279330

280331
// refuseSymlinkComponents walks rel one path segment at a time

internal/backup/checksums_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,63 @@ func TestSplitChecksumLine_RejectsDanglingEscape(t *testing.T) {
393393
}
394394
}
395395

396+
// TestVerifyChecksums_RejectsEmptyChecksumsFile is the regression
397+
// for codex r5 P2 on PR #810: a CHECKSUMS file with zero parsed
398+
// rows (empty, blank lines only) previously returned nil, hiding
399+
// a producer-side corruption. The dump tree is non-empty by
400+
// construction (WriteChecksums lists every regular file), so an
401+
// empty CHECKSUMS is always a signal.
402+
func TestVerifyChecksums_RejectsEmptyChecksumsFile(t *testing.T) {
403+
t.Parallel()
404+
cases := []struct {
405+
name string
406+
body string
407+
}{
408+
{"empty file", ""},
409+
{"blank lines only", "\n\n\n"},
410+
{"trailing newline only", "\n"},
411+
}
412+
for _, tc := range cases {
413+
t.Run(tc.name, func(t *testing.T) {
414+
t.Parallel()
415+
root := t.TempDir()
416+
mustWrite(t, filepath.Join(root, CHECKSUMSFilename), []byte(tc.body))
417+
err := VerifyChecksums(root)
418+
if err == nil {
419+
t.Fatalf("expected ErrChecksumsEmpty for %q, got nil", tc.body)
420+
}
421+
if !errors.Is(err, ErrChecksumsEmpty) {
422+
t.Fatalf("err = %v, want ErrChecksumsEmpty", err)
423+
}
424+
})
425+
}
426+
}
427+
428+
// TestVerifyChecksums_MissingTargetIsMismatch is the regression
429+
// for codex r5 P2 on PR #810: a CHECKSUMS line whose listed file
430+
// was deleted from disk (partial restore / tamper) now surfaces
431+
// as ErrChecksumMismatch — previously the raw fs.ErrNotExist
432+
// bubbled up through refuseSymlinkComponents and bypassed the
433+
// typed checksum-failure path callers branch on.
434+
func TestVerifyChecksums_MissingTargetIsMismatch(t *testing.T) {
435+
t.Parallel()
436+
root := t.TempDir()
437+
mustWrite(t, filepath.Join(root, "a.txt"), []byte("hello"))
438+
if err := WriteChecksums(root); err != nil {
439+
t.Fatalf("WriteChecksums: %v", err)
440+
}
441+
if err := os.Remove(filepath.Join(root, "a.txt")); err != nil {
442+
t.Fatalf("rm a.txt: %v", err)
443+
}
444+
err := VerifyChecksums(root)
445+
if err == nil {
446+
t.Fatalf("expected ErrChecksumMismatch for missing target, got nil")
447+
}
448+
if !errors.Is(err, ErrChecksumMismatch) {
449+
t.Fatalf("err = %v, want ErrChecksumMismatch", err)
450+
}
451+
}
452+
396453
// TestVerifyChecksums_StreamsLargeFile pins the bufio.Scanner path
397454
// — the previous implementation slurped the entire CHECKSUMS file
398455
// via os.ReadFile, which OOMs on large dump trees (gemini

0 commit comments

Comments
 (0)