From c22d0afe998db1dcfb87366acf3734f8cba3f1c8 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 1 Aug 2026 19:00:50 +0200 Subject: [PATCH 1/3] fix(dv): reject trailing bitmap data --- table/dv/roaring_bitmap.go | 3 +++ table/dv/roaring_bitmap_test.go | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/table/dv/roaring_bitmap.go b/table/dv/roaring_bitmap.go index e606773a9..20cbfc818 100644 --- a/table/dv/roaring_bitmap.go +++ b/table/dv/roaring_bitmap.go @@ -179,6 +179,9 @@ func DeserializeRoaringPositionBitmap(data []byte) (*RoaringPositionBitmap, erro lastKey = key hasLastKey = true } + if r.Len() != 0 { + return nil, fmt.Errorf("trailing data after bitmaps: %d bytes", r.Len()) + } return b, nil } diff --git a/table/dv/roaring_bitmap_test.go b/table/dv/roaring_bitmap_test.go index bccc7e9c7..d6670b653 100644 --- a/table/dv/roaring_bitmap_test.go +++ b/table/dv/roaring_bitmap_test.go @@ -148,6 +148,20 @@ func TestDeserializeRoaringBitmapTruncatedAfterKey(t *testing.T) { assert.ErrorContains(t, err, "read bitmap for key 0") } +// Why: the outer count fully frames the portable bitmap and extra bytes are +// malformed rather than a forward-compatible extension. +// Condition: append bytes after an otherwise valid empty bitmap. +// Assertion: returns an error containing "trailing data". +func TestDeserializeRoaringBitmapTrailingData(t *testing.T) { + bm := NewRoaringPositionBitmap() + var buf bytes.Buffer + require.NoError(t, bm.Serialize(&buf)) + buf.Write([]byte{0xde, 0xad}) + + _, err := DeserializeRoaringPositionBitmap(buf.Bytes()) + assert.ErrorContains(t, err, "trailing data") +} + // TestKeepMaskBytes pins KeepMaskBytes' contract: a bit-packed []byte where // bit i is 1 iff position i is NOT in the bitmap, sized to ⌈length/8⌉ bytes, // matching Arrow Boolean buffer layout so it can be wrapped without re-packing. From d0fe7f4b3cfa91cbc8a30fda9e3825fa5bd02896 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 1 Aug 2026 19:07:13 +0200 Subject: [PATCH 2/3] test(dv): cover framed non-empty bitmaps --- table/dv/roaring_bitmap_test.go | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/table/dv/roaring_bitmap_test.go b/table/dv/roaring_bitmap_test.go index d6670b653..c5e93ba3e 100644 --- a/table/dv/roaring_bitmap_test.go +++ b/table/dv/roaring_bitmap_test.go @@ -150,16 +150,29 @@ func TestDeserializeRoaringBitmapTruncatedAfterKey(t *testing.T) { // Why: the outer count fully frames the portable bitmap and extra bytes are // malformed rather than a forward-compatible extension. -// Condition: append bytes after an otherwise valid empty bitmap. +// Condition: append bytes after otherwise valid empty and non-empty bitmaps. // Assertion: returns an error containing "trailing data". func TestDeserializeRoaringBitmapTrailingData(t *testing.T) { - bm := NewRoaringPositionBitmap() - var buf bytes.Buffer - require.NoError(t, bm.Serialize(&buf)) - buf.Write([]byte{0xde, 0xad}) + for _, tt := range []struct { + name string + positions []uint64 + }{ + {name: "empty"}, + {name: "non-empty", positions: []uint64{1, (uint64(3) << 32) | 7}}, + } { + t.Run(tt.name, func(t *testing.T) { + bm := NewRoaringPositionBitmap() + for _, position := range tt.positions { + bm.Set(position) + } + var buf bytes.Buffer + require.NoError(t, bm.Serialize(&buf)) + buf.Write([]byte{0xde, 0xad}) - _, err := DeserializeRoaringPositionBitmap(buf.Bytes()) - assert.ErrorContains(t, err, "trailing data") + _, err := DeserializeRoaringPositionBitmap(buf.Bytes()) + assert.ErrorContains(t, err, "trailing data") + }) + } } // TestKeepMaskBytes pins KeepMaskBytes' contract: a bit-packed []byte where From 1fffd72302b1719fbeee94434e7ad68684d7337e Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 1 Aug 2026 19:12:08 +0200 Subject: [PATCH 3/3] test(dv): cover trailing data through DV envelope --- table/dv/deletion_vector_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/table/dv/deletion_vector_test.go b/table/dv/deletion_vector_test.go index 945447fb4..8c1084976 100644 --- a/table/dv/deletion_vector_test.go +++ b/table/dv/deletion_vector_test.go @@ -331,6 +331,23 @@ func TestDeserializeDVWrapsBitmapDecodeError(t *testing.T) { assert.ErrorContains(t, err, "deserialize deletion vector bitmap") } +// Why: a valid DV length and CRC must not hide extra bytes after the framed +// roaring bitmap. +// Condition: append bytes to a valid inner bitmap, then rebuild the envelope +// so its length and CRC both match the malformed payload. +// Assertion: returns a contextual bitmap error containing "trailing data". +func TestDeserializeDVRejectsTrailingBitmapData(t *testing.T) { + bm := NewRoaringPositionBitmap() + bm.Set(7) + var bitmap bytes.Buffer + require.NoError(t, bm.Serialize(&bitmap)) + bitmap.Write([]byte{0xde, 0xad}) + + _, err := DeserializeDV(wrapDVPayloadForTest(bitmap.Bytes()), -1) + assert.ErrorContains(t, err, "deserialize deletion vector bitmap") + assert.ErrorContains(t, err, "trailing data") +} + // Why: manifest metadata and DV content should agree on the number of deleted rows. // Condition: valid DV blob with 5 positions, but expected cardinality is set to 999. // Assertion: returns an error containing "cardinality mismatch".