Skip to content

Commit e09f7d7

Browse files
committed
fix: remove 100-chunk limit and isBlank skip in nullseed
Remove the 100-chunk limit in LongestMatchWith for non-reflink filesystems. Also remove the isBlank optimization that skipped writing zeros entirely. The isBlank skip relied on Truncate providing zeros, but on APFS (macOS) Truncate creates sparse holes. When concurrent assembly workers write adjacent chunks while the null seed skips its region, the file content becomes non-deterministic. Always writing explicit zeros is safe on all platforms and avoids this race condition. Add TestAssembleOver100NullChunks which creates a file with 110 consecutive max-size null chunks sandwiched between random data, verifies SHA-256 of the assembled output.
1 parent 3761d65 commit e09f7d7

2 files changed

Lines changed: 86 additions & 16 deletions

File tree

assemble_test.go

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"context"
66
"crypto/md5"
77
"crypto/rand"
8+
"crypto/sha256"
89
"io"
910
"os"
1011
"path/filepath"
@@ -401,6 +402,81 @@ func join(slices ...[]byte) []byte {
401402
return out
402403
}
403404

405+
// TestAssembleOver100NullChunks verifies that assembly produces a correct file
406+
// when the index contains >100 consecutive null chunks. This reproduces a bug
407+
// where nullChunkSeed.LongestMatchWith capped matches at 100 on non-reflink
408+
// filesystems, causing overflow null chunks to fall through to writeChunk
409+
// where the isBlank optimization silently skipped adjacent non-null data.
410+
func TestAssembleOver100NullChunks(t *testing.T) {
411+
// Build an input file: random data | >100 max-size null chunks | random data.
412+
// The null region must exceed 100 * ChunkSizeMaxDefault to trigger the bug.
413+
randBlock := make([]byte, 4*ChunkSizeMaxDefault)
414+
rand.Read(randBlock)
415+
416+
nullBlock := make([]byte, 110*ChunkSizeMaxDefault) // 110 consecutive null chunks
417+
418+
var input []byte
419+
input = append(input, randBlock...)
420+
input = append(input, nullBlock...)
421+
input = append(input, randBlock...)
422+
423+
// Write the input to a temp file
424+
inFile, err := os.CreateTemp("", "nullchunk-input-*")
425+
require.NoError(t, err)
426+
defer os.Remove(inFile.Name())
427+
_, err = inFile.Write(input)
428+
require.NoError(t, err)
429+
inFile.Close()
430+
431+
// Record the SHA-256 of the original
432+
expectedSum := sha256.Sum256(input)
433+
434+
// Chunk the file to produce an index
435+
index, _, err := IndexFromFile(
436+
context.Background(),
437+
inFile.Name(),
438+
10,
439+
ChunkSizeMinDefault, ChunkSizeAvgDefault, ChunkSizeMaxDefault,
440+
NewProgressBar(""),
441+
)
442+
require.NoError(t, err)
443+
444+
// Verify the index actually contains >100 consecutive null chunks
445+
nullID := NewNullChunk(ChunkSizeMaxDefault).ID
446+
maxRun := 0
447+
run := 0
448+
for _, c := range index.Chunks {
449+
if c.ID == nullID {
450+
run++
451+
if run > maxRun {
452+
maxRun = run
453+
}
454+
} else {
455+
run = 0
456+
}
457+
}
458+
require.Greater(t, maxRun, 100, "test input must produce >100 consecutive null chunks")
459+
460+
// Chop the file into a local store
461+
store := t.TempDir()
462+
s, err := NewLocalStore(store, StoreOptions{})
463+
require.NoError(t, err)
464+
require.NoError(t, ChopFile(context.Background(), inFile.Name(), index.Chunks, s, 10, NewProgressBar("")))
465+
466+
// Assemble into a new (non-existing) file — isBlank=true path
467+
outFile := filepath.Join(t.TempDir(), "output")
468+
_, err = AssembleFile(context.Background(), outFile, index, s, nil,
469+
AssembleOptions{10, InvalidSeedActionBailOut},
470+
)
471+
require.NoError(t, err)
472+
473+
// Compare SHA-256
474+
assembled, err := os.ReadFile(outFile)
475+
require.NoError(t, err)
476+
actualSum := sha256.Sum256(assembled)
477+
require.Equal(t, expectedSum, actualSum, "SHA-256 of assembled file must match original")
478+
}
479+
404480
func readCaibxFile(t *testing.T, indexLocation string) (idx Index) {
405481
is, err := NewLocalIndexStore(filepath.Dir(indexLocation))
406482
require.NoError(t, err)

nullseed.go

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -46,17 +46,12 @@ func (s *nullChunkSeed) LongestMatchWith(chunks []IndexChunk) (int, SeedSegment)
4646
if len(chunks) == 0 {
4747
return 0, nil
4848
}
49-
var (
50-
n int
51-
limit int
52-
)
53-
if !s.canReflink {
54-
limit = 100
55-
}
49+
// No limit needed: when isBlank=true, WriteInto skips without copying.
50+
// When isBlank=false, we must still write zeros to overwrite stale data.
51+
// The previous limit of 100 caused chunks beyond the limit to fall
52+
// through to other code paths, leading to incorrect assembly.
53+
var n int
5654
for _, c := range chunks {
57-
if limit != 0 && limit == n {
58-
break
59-
}
6055
if c.ID != s.id {
6156
break
6257
}
@@ -108,13 +103,12 @@ func (s *nullChunkSection) WriteInto(dst *os.File, offset, length, blocksize uin
108103
return 0, 0, fmt.Errorf("unable to copy %d bytes to %s : wrong size", length, dst.Name())
109104
}
110105

111-
// When cloning isn't available we'd normally have to copy the 0 bytes into
112-
// the target range. But if that's already blank (because it's a new/truncated
113-
// file) there's no need to copy 0 bytes.
106+
// Always write explicit zeros rather than relying on truncation.
107+
// On some filesystems (e.g., APFS), Truncate creates sparse holes that
108+
// can interact poorly with concurrent assembly workers, leading to
109+
// non-deterministic corruption. Writing zeros explicitly is safe on all
110+
// platforms and avoids the race.
114111
if !s.canReflink {
115-
if isBlank {
116-
return 0, 0, nil
117-
}
118112
return s.copy(dst, offset, s.Size())
119113
}
120114
return s.clone(dst, offset, length, blocksize)

0 commit comments

Comments
 (0)