From 77644159feb26c5b30ac4b197b130c842c3374bf Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 31 Aug 2026 17:02:53 -0400 Subject: [PATCH 1/3] feat(sdk): make the zipstream clock injectable for deterministic ZIP output zipstream stamped ZIP header and segment-metadata timestamps straight from time.Now, so archive bytes could never be compared byte-for-byte across runs. Config gains a Now func() time.Time, defaulted to time.Now and overridable via WithClock; NewSegmentMetadata takes the time source explicitly and SegmentEntry.Written stamps from it. Now is an exported field on an exported Config and Option is a bare func(*Config), so an option is free to nil it out even though WithClock will not. applyOptions restores the default rather than letting the first header stamp panic. Injecting a clock also makes the MS-DOS date encoder reachable with years it cannot represent. The year is a 7-bit offset from 1980, so an out-of-range value wrapped through the uint16 conversion into a plausible but wrong date, silently and with no error: the zero time.Time landed on 2049-01-01 and the Unix epoch on 2098-01-01. The two copies of the encoder, one for the local file header and one for the central directory, are now a single msDosTimeDate that clamps to 1980..2107, so fixing one copy cannot leave the other wrapped. No behavior change for existing callers: sdk/tdf.go constructs the writer without WithClock and keeps time.Now, which is always in range. Signed-off-by: David Mihalcik From 4e247fc63464e30fbf075707593d4ab0bbfa4a63 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 31 Aug 2026 17:03:56 -0400 Subject: [PATCH 2/3] fix(sdk): reject a zipstream write set that omits segment 0 Only segment 0 emits the payload's ZIP local file header, and Finalize computes every offset it records -- central directory, data descriptor, end-of-central-directory -- as though that header sits at the front of the assembled stream. A write set that skips index 0 therefore produced a structurally corrupt archive whose trailer pointed a reader past the end of its own buffer. IsComplete cannot catch this: Order is derived from whatever indices arrived, so {1, 2} is internally consistent. The check has to stand on its own, and it has to survive CleanupSegment(0) dropping the header after the fact. Sparse indices remain legal -- a caller mapping S3 multipart uploads onto segments may write 0, 1, 5000 -- so this only requires that the set starts at 0, not that it is contiguous. Behavior change: Finalize now returns ErrNoSegmentZero where it previously returned a corrupt archive. No in-repo caller is affected; sdk/tdf.go writes segments sequentially from 0. Signed-off-by: David Mihalcik --- sdk/internal/zipstream/segment_writer.go | 29 ++++- sdk/internal/zipstream/segment_writer_test.go | 106 ++++++++++++++++++ sdk/internal/zipstream/writer.go | 15 ++- 3 files changed, 147 insertions(+), 3 deletions(-) diff --git a/sdk/internal/zipstream/segment_writer.go b/sdk/internal/zipstream/segment_writer.go index b6f1d7191f..2604b286ef 100644 --- a/sdk/internal/zipstream/segment_writer.go +++ b/sdk/internal/zipstream/segment_writer.go @@ -126,6 +126,29 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte, default: } + // Nothing arrived at all: report the general incomplete-input error + // rather than the segment-0-specific one below. + if len(sw.metadata.Segments) == 0 { + return nil, &Error{Op: "finalize", Type: "segment", Err: ErrSegmentMissing} + } + + // Only segment 0 emits the payload's local file header, and every offset + // recorded below is measured from it: without it the manifest entry, the + // central directory, and the EOCD all overshoot by headerSize. The result + // is a corrupt archive rather than a clean failure -- under Zip64Always + // the trailer points past the end of the buffer, while under Zip64Auto it + // lands mid-archive, where some readers parse the manifest happily and + // only choke on the payload. + // + // This has to run before the order derivation below: Order is derived + // once and kept, so a caller that supplies segment 0 and retries must not + // inherit an order that already excluded it. IsComplete cannot catch the + // absence either, since that derived order is self-consistent by + // construction. + if _, ok := sw.metadata.Segments[0]; !ok { + return nil, &Error{Op: "finalize", Type: "segment", Err: ErrNoSegmentZero} + } + // If no explicit order was provided, derive order from present indices (sorted). if len(sw.metadata.Order) == 0 { order := make([]int, 0, len(sw.metadata.Segments)) @@ -223,8 +246,10 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte, } // CleanupSegment removes the presence marker for a segment index. Since payload -// bytes are not retained, this only affects metadata tracking. Calling this -// before Finalize will cause IsComplete() to fail for that index. +// bytes are not retained, this only affects metadata tracking. Finalize infers +// segment order from whichever indices survive, so a cleaned-up index drops out +// of that order rather than making IsComplete fail; only index 0 is rejected, +// with ErrNoSegmentZero. Size accounting on payloadEntry is not rolled back. func (sw *segmentWriter) CleanupSegment(index int) error { sw.mu.Lock() defer sw.mu.Unlock() diff --git a/sdk/internal/zipstream/segment_writer_test.go b/sdk/internal/zipstream/segment_writer_test.go index c57830d858..c7f0d15675 100644 --- a/sdk/internal/zipstream/segment_writer_test.go +++ b/sdk/internal/zipstream/segment_writer_test.go @@ -362,6 +362,112 @@ func TestSegmentWriter_AllowsGapsOnFinalize(t *testing.T) { writer.Close() } +func TestSegmentWriter_FinalizeRequiresSegmentZero(t *testing.T) { + // Gaps are fine, but the set has to start at 0: only segment 0 emits + // the payload local file header, and Finalize sizes every offset it + // records as though that header were at the front of the stream. + writer := NewSegmentTDFWriter(1) + ctx := t.Context() + + _, err := writer.WriteSegment(ctx, 1, 5, crc32.ChecksumIEEE([]byte("first"))) + require.NoError(t, err) + + _, err = writer.WriteSegment(ctx, 2, 6, crc32.ChecksumIEEE([]byte("second"))) + require.NoError(t, err) + + _, err = writer.Finalize(ctx, []byte("manifest")) + require.ErrorIs(t, err, ErrNoSegmentZero) + + writer.Close() +} + +func TestSegmentWriter_CleanupSegmentZeroBlocksFinalize(t *testing.T) { + // Dropping segment 0 after the fact is invisible to IsComplete -- + // the inferred order becomes [1], which is internally consistent -- + // so the header check has to stand on its own. + writer := NewSegmentTDFWriter(2) + ctx := t.Context() + + _, err := writer.WriteSegment(ctx, 0, 5, crc32.ChecksumIEEE([]byte("first"))) + require.NoError(t, err) + + _, err = writer.WriteSegment(ctx, 1, 6, crc32.ChecksumIEEE([]byte("second"))) + require.NoError(t, err) + + require.NoError(t, writer.CleanupSegment(0)) + + _, err = writer.Finalize(ctx, []byte("manifest")) + require.ErrorIs(t, err, ErrNoSegmentZero) + require.NotErrorIs(t, err, ErrSegmentMissing, "the remaining segments are complete; only the header is gone") + + writer.Close() +} + +func TestSegmentWriter_FinalizeAfterSupplyingSegmentZero(t *testing.T) { + // ErrNoSegmentZero invites the caller to write segment 0 and try again, + // so the retry has to produce a correct archive. Finalize derives the + // segment order once and keeps it: if the check ran after that + // derivation, this second Finalize would succeed against an order that + // still omitted 0, combining a CRC over two of the three segments. + writer := NewSegmentTDFWriter(3) + ctx := t.Context() + + segments := [][]byte{[]byte("first"), []byte("second"), []byte("third")} + + for _, index := range []int{1, 2} { + data := segments[index] + _, err := writer.WriteSegment(ctx, index, uint64(len(data)), crc32.ChecksumIEEE(data)) + require.NoError(t, err) + } + + _, err := writer.Finalize(ctx, []byte("manifest")) + require.ErrorIs(t, err, ErrNoSegmentZero) + + headerBytes, err := writer.WriteSegment(ctx, 0, uint64(len(segments[0])), crc32.ChecksumIEEE(segments[0])) + require.NoError(t, err, "the writer stays usable after ErrNoSegmentZero") + require.NotEmpty(t, headerBytes, "segment 0 carries the payload local file header") + + var archive []byte + archive = append(archive, headerBytes...) + for _, data := range segments { + archive = append(archive, data...) + } + + finalBytes, err := writer.Finalize(ctx, []byte("manifest")) + require.NoError(t, err, "Finalize should succeed once segment 0 arrives") + archive = append(archive, finalBytes...) + + zipReader, err := zip.NewReader(bytes.NewReader(archive), int64(len(archive))) + require.NoError(t, err, "retry should produce a readable ZIP") + + payloadFile := findFileByName(zipReader, TDFPayloadFileName) + require.NotNil(t, payloadFile) + + payloadReader, err := payloadFile.Open() + require.NoError(t, err) + defer payloadReader.Close() + + // Reading through archive/zip validates the recorded CRC against the + // bytes actually present, which is what a stale order would break. + content, err := io.ReadAll(payloadReader) + require.NoError(t, err, "payload CRC must cover every segment, including 0") + assert.Equal(t, bytes.Join(segments, nil), content) + + writer.Close() +} + +func TestSegmentWriter_FinalizeWithoutAnySegments(t *testing.T) { + // No segments at all is incomplete input, not a missing-header problem: + // the general error stays reachable and keeps its distinct meaning. + writer := NewSegmentTDFWriter(2) + + _, err := writer.Finalize(t.Context(), []byte("manifest")) + require.ErrorIs(t, err, ErrSegmentMissing) + require.NotErrorIs(t, err, ErrNoSegmentZero) + + writer.Close() +} + func TestSegmentWriter_CleanupSegment(t *testing.T) { // Test memory cleanup functionality writer := NewSegmentTDFWriter(3) diff --git a/sdk/internal/zipstream/writer.go b/sdk/internal/zipstream/writer.go index 11eea8a5b5..74de6cbe27 100644 --- a/sdk/internal/zipstream/writer.go +++ b/sdk/internal/zipstream/writer.go @@ -24,9 +24,21 @@ type Writer interface { type SegmentWriter interface { Writer WriteSegment(ctx context.Context, index int, size uint64, crc32 uint32) ([]byte, error) + // Finalize writes the trailer (data descriptor, manifest, central + // directory) for the segments recorded so far. Segment 0 must be among + // them: it carries the payload local file header that every recorded + // offset is measured from. Finalize returns ErrNoSegmentZero when index 0 + // was never written or was cleaned up, and ErrSegmentMissing when no + // segments were written at all. Gaps between the remaining indices are + // accepted; order is inferred by sorting whichever indices are present. Finalize(ctx context.Context, manifest []byte) ([]byte, error) // CleanupSegment removes the presence marker for a segment index. - // Calling this before Finalize will cause IsComplete() to fail for that index. + // Finalize infers segment order from whichever indices survive, so a + // cleaned-up index simply drops out of that order rather than making + // Finalize report ErrSegmentMissing. Index 0 is the exception: Finalize + // rejects its absence with ErrNoSegmentZero. Payload size accounting is + // not rolled back, so finalizing after a cleanup declares more payload + // bytes than the caller has to assemble. CleanupSegment(index int) error } @@ -52,6 +64,7 @@ var ( ErrOutOfOrder = errors.New("segment out of order") ErrDuplicateSegment = errors.New("duplicate segment already written") ErrSegmentMissing = errors.New("segment missing") + ErrNoSegmentZero = errors.New("segment 0 missing; it carries the payload local file header") ErrInvalidSize = errors.New("invalid size") ErrZip64Required = errors.New("ZIP64 required but disabled (Zip64Never)") ) From a7b141e1ce2ac2ceea40c105aaacb9f6f80b1568 Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Wed, 2 Sep 2026 12:18:00 -0400 Subject: [PATCH 3/3] fix(sdk): roll back zipstream size accounting on CleanupSegment CleanupSegment dropped a segment's presence marker but left its bytes in payloadEntry.Size and CompressedSize. Finalize then combined a CRC over the survivors while sizing every offset as though the removed segment were still there, so the trailer described a payload the caller could not assemble: archive/zip opens the result, reads the manifest happily, and fails the payload with a checksum error or a negative offset. That is the same corruption this branch already rejects for index 0. Undo the size contribution alongside the presence marker, making a cleaned-up index indistinguishable from one that was never written -- which sparse write sets already allow. The presentCount > 0 clamp goes away with it; we now only decrement when an entry was actually found, so the guarded state is unreachable and would have masked a real accounting bug. Docs: Finalize returns ErrSegmentMissing when no segments *remain*, not only when none were written -- cleaning up the last one lands there too. The experimental/tdf Finalize doc claimed gaps in segment indices cause failure; they are legal and tested, while the real new failure (a set that omits index 0) went unlisted. The CleanupSegment contract now lives on the interface rather than being duplicated and divergent across it and the implementation. Signed-off-by: David Mihalcik --- sdk/experimental/tdf/writer.go | 5 +- sdk/internal/zipstream/segment_writer.go | 33 ++++++---- sdk/internal/zipstream/segment_writer_test.go | 66 +++++++++++++++++++ sdk/internal/zipstream/writer.go | 19 +++--- 4 files changed, 101 insertions(+), 22 deletions(-) diff --git a/sdk/experimental/tdf/writer.go b/sdk/experimental/tdf/writer.go index 02d2f8af53..2a58af6c87 100644 --- a/sdk/experimental/tdf/writer.go +++ b/sdk/experimental/tdf/writer.go @@ -341,7 +341,10 @@ func (w *Writer) WriteSegment(ctx context.Context, index int, data []byte) (*Seg // // Error conditions: // - ErrAlreadyFinalized: Finalize already called -// - Missing segments: Gaps in segment indices (e.g., segments 0,1,3 written but 2 missing) +// - Missing segment 0: Index 0 carries the payload's ZIP local file header, which +// every recorded offset is measured from, so a write set that omits it is +// rejected. Gaps between the remaining indices are legal (e.g., segments 0,1,3 +// with 2 missing); order is inferred by sorting whichever indices are present. // - Key splitting failures: Invalid attributes or KAS configuration // - Manifest generation errors: JSON marshaling failures // - Archive finalization errors: ZIP structure generation failures diff --git a/sdk/internal/zipstream/segment_writer.go b/sdk/internal/zipstream/segment_writer.go index 2604b286ef..18b35b62ce 100644 --- a/sdk/internal/zipstream/segment_writer.go +++ b/sdk/internal/zipstream/segment_writer.go @@ -162,7 +162,10 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte, } } - // Verify all segments are present + // Verify all segments are present. Unreachable with an order derived + // above -- that order is built from the present indices, so it is + // complete by construction, and the empty set already returned. Kept for + // a future caller that supplies an explicit order. if !sw.metadata.IsComplete() { return nil, &Error{Op: "finalize", Type: "segment", Err: ErrSegmentMissing} } @@ -245,23 +248,29 @@ func (sw *segmentWriter) Finalize(ctx context.Context, manifest []byte) ([]byte, return buffer.Bytes(), nil } -// CleanupSegment removes the presence marker for a segment index. Since payload -// bytes are not retained, this only affects metadata tracking. Finalize infers -// segment order from whichever indices survive, so a cleaned-up index drops out -// of that order rather than making IsComplete fail; only index 0 is rejected, -// with ErrNoSegmentZero. Size accounting on payloadEntry is not rolled back. +// CleanupSegment implements SegmentWriter. Payload bytes are never retained, so +// this only rolls back the metadata and size accounting the segment contributed. func (sw *segmentWriter) CleanupSegment(index int) error { sw.mu.Lock() defer sw.mu.Unlock() - // Remove segment from unprocessed map (no-op if already processed or not found) - if _, ok := sw.metadata.Segments[index]; ok { - delete(sw.metadata.Segments, index) - if sw.metadata.presentCount > 0 { - sw.metadata.presentCount-- - } + // No-op if the index was never written or was already cleaned up. + seg, ok := sw.metadata.Segments[index] + if !ok { + return nil } + delete(sw.metadata.Segments, index) + sw.metadata.presentCount-- + + // Undo everything the segment contributed, so that a cleaned-up index is + // indistinguishable from one that was never written. Leaving the sizes + // behind would make Finalize describe a payload larger than the one the + // caller can assemble, and the offsets it records would overshoot. + sw.metadata.TotalSize -= seg.Size + sw.payloadEntry.Size -= seg.Size + sw.payloadEntry.CompressedSize -= seg.Size + return nil } diff --git a/sdk/internal/zipstream/segment_writer_test.go b/sdk/internal/zipstream/segment_writer_test.go index c7f0d15675..a4319e66f5 100644 --- a/sdk/internal/zipstream/segment_writer_test.go +++ b/sdk/internal/zipstream/segment_writer_test.go @@ -468,6 +468,72 @@ func TestSegmentWriter_FinalizeWithoutAnySegments(t *testing.T) { writer.Close() } +func TestSegmentWriter_CleanupOnlySegmentZero(t *testing.T) { + // Cleaning up the last remaining segment leaves nothing to finalize, so + // this reports the general incomplete-input error rather than the + // segment-0-specific one -- "nothing to assemble" is the more useful + // diagnosis than "the header is gone". + writer := NewSegmentTDFWriter(1) + ctx := t.Context() + + _, err := writer.WriteSegment(ctx, 0, 5, crc32.ChecksumIEEE([]byte("first"))) + require.NoError(t, err) + + require.NoError(t, writer.CleanupSegment(0)) + + _, err = writer.Finalize(ctx, []byte("manifest")) + require.ErrorIs(t, err, ErrSegmentMissing) + require.NotErrorIs(t, err, ErrNoSegmentZero) + + writer.Close() +} + +func TestSegmentWriter_CleanupSegmentRollsBackSizeAccounting(t *testing.T) { + // A cleaned-up index has to become indistinguishable from one that was + // never written, which sparse write sets already allow. Without the + // rollback the recorded sizes still cover the removed segment while the + // CRC covers only the survivors, and Finalize emits a trailer describing + // a payload the caller cannot produce. + writer := NewSegmentTDFWriter(3) + ctx := t.Context() + + segments := [][]byte{[]byte("first"), []byte("second"), []byte("third")} + + var archive []byte + for index, data := range segments { + headerBytes, err := writer.WriteSegment(ctx, index, uint64(len(data)), crc32.ChecksumIEEE(data)) + require.NoError(t, err) + archive = append(archive, headerBytes...) + if index != 1 { + archive = append(archive, data...) + } + } + + require.NoError(t, writer.CleanupSegment(1)) + + finalBytes, err := writer.Finalize(ctx, []byte("manifest")) + require.NoError(t, err) + archive = append(archive, finalBytes...) + + zipReader, err := zip.NewReader(bytes.NewReader(archive), int64(len(archive))) + require.NoError(t, err, "offsets must describe the payload the caller actually assembled") + + payloadFile := findFileByName(zipReader, TDFPayloadFileName) + require.NotNil(t, payloadFile) + + payloadReader, err := payloadFile.Open() + require.NoError(t, err) + defer payloadReader.Close() + + // Reading through archive/zip validates the recorded CRC against the + // bytes present, which is what stale size accounting would break. + content, err := io.ReadAll(payloadReader) + require.NoError(t, err, "payload CRC must cover exactly the surviving segments") + assert.Equal(t, []byte("firstthird"), content) + + writer.Close() +} + func TestSegmentWriter_CleanupSegment(t *testing.T) { // Test memory cleanup functionality writer := NewSegmentTDFWriter(3) diff --git a/sdk/internal/zipstream/writer.go b/sdk/internal/zipstream/writer.go index 74de6cbe27..774e1c9488 100644 --- a/sdk/internal/zipstream/writer.go +++ b/sdk/internal/zipstream/writer.go @@ -29,16 +29,17 @@ type SegmentWriter interface { // them: it carries the payload local file header that every recorded // offset is measured from. Finalize returns ErrNoSegmentZero when index 0 // was never written or was cleaned up, and ErrSegmentMissing when no - // segments were written at all. Gaps between the remaining indices are - // accepted; order is inferred by sorting whichever indices are present. + // segments remain at all -- none were written, or every one was cleaned + // up. Gaps between the remaining indices are accepted; order is inferred + // by sorting whichever indices are present. Finalize(ctx context.Context, manifest []byte) ([]byte, error) - // CleanupSegment removes the presence marker for a segment index. - // Finalize infers segment order from whichever indices survive, so a - // cleaned-up index simply drops out of that order rather than making - // Finalize report ErrSegmentMissing. Index 0 is the exception: Finalize - // rejects its absence with ErrNoSegmentZero. Payload size accounting is - // not rolled back, so finalizing after a cleanup declares more payload - // bytes than the caller has to assemble. + // CleanupSegment drops a segment index, rolling back both its presence + // marker and the payload size it contributed. A cleaned-up index becomes + // indistinguishable from one that was never written: it falls out of the + // order Finalize infers, exactly as a gap in the write set would, and the + // caller must leave its bytes out of the assembled archive. Index 0 is the + // exception -- Finalize rejects its absence with ErrNoSegmentZero. + // Cleaning up an index that was never written is a no-op. CleanupSegment(index int) error }