amadan
host: the prepared build checks a file's size before reading it and bounds every read (Task 6 fix round 1, finding 5)
The walk stats first: a file over hostfiles.MaxFileBytes is recorded as skipped (Prepared.StageSkipped, no object) without a read. Every read goes through readFileBounded, an io.LimitReader at the limit plus one byte, so a file that grows past the limit while it is read is recorded as skipped too and is never held whole in memory. Tests: a sparse file over the limit is skipped with no read of it; the bounded read reports a file over a small limit with at most limit+1 bytes read and reads one at the limit whole; StageSkipped names no object.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4 files changed,
+136
−1
internal/host/movesfiles.go+35 −1internal/host/movesfiles_test.go+62 −0internal/hostfiles/prepared.go+15 −0internal/hostfiles/prepared_test.go+24 −0
diff --git a/internal/host/movesfiles.go b/internal/host/movesfiles.go| index 2e3fb96f0..58acc08f4 100644 |
| --- a/internal/host/movesfiles.go |
| +++ b/internal/host/movesfiles.go |
| @@ -8,6 +8,7 @@ import ( |
| "encoding/json" |
| "errors" |
| "fmt" |
| + "io" |
| "io/fs" |
| "os" |
| "path/filepath" |
| @@ -250,10 +251,20 @@ func buildPrepared(ctx context.Context, s release.Store, location, generation, o |
| if err != nil { |
| return err |
| } |
| - body, err := os.ReadFile(path) |
| + // The size before any read, and the read bounded at the limit |
| + // plus one byte (fix round 1, finding 5): a file over the limit, by |
| + // its stat or by growing while it is read, is recorded as skipped |
| + // and never held whole in memory. |
| + if info.Size() > hostfiles.MaxFileBytes { |
| + return p.StageSkipped(filepath.ToSlash(rel), movesFilesTooLarge(info.Size())) |
| + } |
| + body, tooLarge, err := readFileBounded(path, hostfiles.MaxFileBytes) |
| if err != nil { |
| return err |
| } |
| + if tooLarge { |
| + return p.StageSkipped(filepath.ToSlash(rel), movesFilesTooLarge(int64(len(body)))) |
| + } |
| return p.Stage(ctx, filepath.ToSlash(rel), body, uint32(info.Mode().Perm())) |
| }) |
| if err != nil { |
| @@ -282,6 +293,29 @@ func buildPrepared(ctx context.Context, s release.Store, location, generation, o |
| return preparedManifest{Key: hostfiles.ManifestKey(prefix), SHA256: hex.EncodeToString(sum[:]), Version: version, Op: op, Stamp: stamp}, nil |
| } |
| +// movesFilesTooLarge is a skipped entry's reason for a file over the |
| +// limit; size is what the stat or the bounded read saw. |
| +func movesFilesTooLarge(size int64) string { |
| + return fmt.Sprintf("over the %d-byte limit (%d bytes or more)", hostfiles.MaxFileBytes, size) |
| +} |
| + |
| +// readFileBounded reads path whole when it holds at most limit bytes. It |
| +// reads at most limit+1 bytes however large the file is or becomes while |
| +// it is read; tooLarge reports that the file exceeded the limit, and the |
| +// bytes returned with it are then not the whole file. A test seam. |
| +var readFileBounded = func(path string, limit int64) ([]byte, bool, error) { |
| + f, err := os.Open(path) |
| + if err != nil { |
| + return nil, false, err |
| + } |
| + defer f.Close() |
| + body, err := io.ReadAll(io.LimitReader(f, limit+1)) |
| + if err != nil { |
| + return nil, false, err |
| + } |
| + return body, int64(len(body)) > limit, nil |
| +} |
| + |
| // movesFilesChain is the admitted predecessor chain whose manifests are in |
| // this region's bucket, newest first: the holder's generation, then each |
| // cold admission's predecessor while it is a named generation in this |
diff --git a/internal/host/movesfiles_test.go b/internal/host/movesfiles_test.go| index 9d77aa547..b7a08cc75 100644 |
| --- a/internal/host/movesfiles_test.go |
| +++ b/internal/host/movesfiles_test.go |
| @@ -321,6 +321,68 @@ func TestPreparedManifestRefusesMissingInputs(t *testing.T) { |
| } |
| } |
| +// The build never reads a file past the limit (fix round 1, finding 5): |
| +// a file whose size is over hostfiles.MaxFileBytes is recorded as skipped |
| +// from its stat alone, and every read is bounded at the limit plus one |
| +// byte, so a file that grows while it is read is recorded as skipped |
| +// rather than read whole. A sparse file stands in for the oversized one. |
| +func TestPreparedBuildBoundsEveryRead(t *testing.T) { |
| + x := newFilesFixture(t) |
| + ctx := context.Background() |
| + dir := filepath.Join(x.dir, filesHost) |
| + if err := os.MkdirAll(dir, 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.WriteFile(filepath.Join(dir, "a.txt"), []byte("A"), 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + big := filepath.Join(dir, "big.bin") |
| + if err := os.WriteFile(big, nil, 0o640); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.Truncate(big, hostfiles.MaxFileBytes+1); err != nil { |
| + t.Fatal(err) |
| + } |
| + x.writeEnv("K=1\n") |
| + reads := 0 |
| + orig := readFileBounded |
| + readFileBounded = func(path string, limit int64) ([]byte, bool, error) { |
| + if filepath.Base(path) == "big.bin" { |
| + reads++ |
| + } |
| + return orig(path, limit) |
| + } |
| + defer func() { readFileBounded = orig }() |
| + if _, err := x.f.BuildPrepared(ctx, filesLoc, "g1", "g1", 7, dir, x.envPath(), true, hostfiles.WakeState{V: 1, Host: filesHost, Timers: []hostfiles.WakeTimer{}}); err != nil { |
| + t.Fatal(err) |
| + } |
| + m, _ := x.manifest("g1") |
| + if e := m.Entries["big.bin"]; e.Skipped == "" || e.Object != "" { |
| + t.Fatalf("the oversized file was not recorded as skipped: %+v", e) |
| + } |
| + if reads != 0 { |
| + t.Fatalf("the oversized file was read %d times", reads) |
| + } |
| + if e := m.Entries["a.txt"]; e.SHA256 != sha([]byte("A")) { |
| + t.Fatalf("a.txt: %+v", e) |
| + } |
| + // The bounded read itself: a file larger than the limit is reported as |
| + // too large with at most limit+1 bytes read; one at the limit is read |
| + // whole. |
| + small := filepath.Join(x.dir, "small") |
| + if err := os.WriteFile(small, []byte("0123456789"), 0o600); err != nil { |
| + t.Fatal(err) |
| + } |
| + body, tooLarge, err := orig(small, 4) |
| + if err != nil || !tooLarge || len(body) > 5 { |
| + t.Fatalf("over the limit: %q %v %v", body, tooLarge, err) |
| + } |
| + body, tooLarge, err = orig(small, 10) |
| + if err != nil || tooLarge || string(body) != "0123456789" { |
| + t.Fatalf("at the limit: %q %v %v", body, tooLarge, err) |
| + } |
| +} |
| + |
| // A replay of the build (a crash after the objects, before or after the |
| // manifest) reuses every object it finds by digest and the manifest by |
| // key: the same binding, one version of each object, no new object. |
diff --git a/internal/hostfiles/prepared.go b/internal/hostfiles/prepared.go| index 8a2a78163..8a2ab7723 100644 |
| --- a/internal/hostfiles/prepared.go |
| +++ b/internal/hostfiles/prepared.go |
| @@ -146,6 +146,21 @@ func (p *Prepared) Stage(ctx context.Context, path string, body []byte, mode uin |
| return nil |
| } |
| +// StageSkipped records that path exists but was not published, and why: |
| +// a file the builder did not read because its size is over the limit (the |
| +// caller decides from the stat, before any read). |
| +func (p *Prepared) StageSkipped(path, why string) error { |
| + clean, err := CleanPath(path) |
| + if err != nil { |
| + return err |
| + } |
| + if p.spent { |
| + return errors.New("hostfiles: prepared manifest already committed") |
| + } |
| + p.man.Entries[clean] = Entry{Seq: 0, Skipped: why} |
| + return nil |
| +} |
| + |
| // StagePlatform records a platform-managed item (the env file, the wake |
| // state). |
| func (p *Prepared) StagePlatform(ctx context.Context, name string, body []byte) error { |
diff --git a/internal/hostfiles/prepared_test.go b/internal/hostfiles/prepared_test.go| index 326a7c8d5..dbcd5f833 100644 |
| --- a/internal/hostfiles/prepared_test.go |
| +++ b/internal/hostfiles/prepared_test.go |
| @@ -173,6 +173,30 @@ func TestPreparedReplayReusesObjects(t *testing.T) { |
| } |
| } |
| +// StageSkipped records a path the builder did not read (a file over the |
| +// limit, by its size): an entry with the reason, no object, at sequence 0. |
| +func TestPreparedStageSkippedNamesNoObject(t *testing.T) { |
| + ctx, s := context.Background(), release.NewDirStore(t.TempDir()) |
| + p := prepared(t, s, pfx) |
| + if err := p.StageSkipped("big.bin", "too large"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := p.StageSkipped("../out", "x"); err == nil { |
| + t.Fatal("staged a path outside the root") |
| + } |
| + m, _, _, err := p.Commit(ctx) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if e := m.Entries["big.bin"]; e.Skipped != "too large" || e.Object != "" || e.Seq != 0 { |
| + t.Fatalf("skipped entry: %+v", e) |
| + } |
| + keys, err := s.List(ctx, pfx) |
| + if err != nil || len(keys) != 1 { |
| + t.Fatalf("objects for a skipped entry: %v %v", keys, err) |
| + } |
| +} |
| + |
| // NewFromCopy seeds a publisher for a generation that has no manifest yet |
| // with a predecessor's validated manifest: every entry keeps its |
| // origin-qualified object, the sequence continues from the copy, and the |