From 2957ffed956fb5f562d193bb58886bc57074e190 Mon Sep 17 00:00:00 2001 From: Daniel Lemire Date: Mon, 19 Oct 2026 17:24:40 -0400 Subject: [PATCH] Validate: reject runs past the container end, accept non-minimal run containers runContainer16.validate() did not check that a run ends within the container: interval16.last() wraps around, so a run from 65000 to 67000 passed, and Maximum() then returned 1464, which Contains denied. Reject it with the new ErrRunIntervalOutOfRange. validate() also rejected a run container that was not the smallest encoding of its values, although AddOffset produces such containers from valid bitmaps. Drop that rule, as CRoaring does and as RoaringBitmap/RoaringBitmap#884 does for Java. ErrRunIntervalSize is kept, deprecated, for compatibility. The deserialization tests named "Corrupts Run Length" did not run-optimize their bitmaps, so they corrupted array containers, and passed only because the harness checked for any error. They now use run containers and check the run end, and both harnesses check the expected error with ErrorIs. --- roaring.go | 2 ++ roaring_test.go | 45 ++++++++++++++++++++++++++++-------- runcontainer.go | 43 ++++++++++++---------------------- runcontainer_test.go | 15 +++++++++--- serialization_frozen_test.go | 24 +++++++++++-------- 5 files changed, 80 insertions(+), 49 deletions(-) diff --git a/roaring.go b/roaring.go index 129034e3..8f80e838 100644 --- a/roaring.go +++ b/roaring.go @@ -2476,6 +2476,8 @@ func (rb *Bitmap) Describe() { // Validate checks if the bitmap is internally consistent. // You may call it after deserialization to check that the bitmap is valid. // This function returns an error if the bitmap is invalid, nil otherwise. +// It does not check that each container uses the most compact encoding: +// call RunOptimize for that. func (rb *Bitmap) Validate() error { return rb.highlowcontainer.validate() } diff --git a/roaring_test.go b/roaring_test.go index 04c9abdd..3d6ed1df 100644 --- a/roaring_test.go +++ b/roaring_test.go @@ -3376,28 +3376,34 @@ func TestBitMapValidationFromDeserialization(t *testing.T) { err error }{ { - name: "Corrupts Run Length vs Num Runs", + name: "Last Run Extends Past Container End", loader: func(bm *Bitmap) { bm.AddRange(0, 2) bm.AddRange(4, 6) bm.AddRange(8, 100) + bm.RunOptimize() // one run container }, corruptor: func(s []byte) { - // 21 is the length of the run of the last run/range - // Shortening causes interval sum to be to short - s[21] = 1 + // 21-22 is the length of the last run, which starts at 8: + // 65535 makes it end past 65535 + s[21] = 0xFF + s[22] = 0xFF }, - err: ErrRunIntervalSize, + err: ErrRunIntervalOutOfRange, }, { - name: "Corrupts Run Length", + name: "Run Extends Past Container End", loader: func(bm *Bitmap) { bm.AddRange(100, 110) + bm.RunOptimize() // one run container }, corruptor: func(s []byte) { - s[13] = 0 + // 13-14 is the length of the run, which starts at 100: + // 65535 makes it end past 65535 + s[13] = 0xFF + s[14] = 0xFF }, - err: ErrRunIntervalSize, + err: ErrRunIntervalOutOfRange, }, { name: "Creates Interval Overlap", @@ -3467,7 +3473,7 @@ func TestBitMapValidationFromDeserialization(t *testing.T) { if tt.err == nil { assert.NoError(t, validateErr, "expected validation to succeed when tt.err is nil") } else { - assert.Error(t, validateErr, "expected validation to fail when tt.err is not nil") + assert.ErrorIs(t, validateErr, tt.err) } corruptedDeserializedBitMap = NewBitmap() @@ -4184,3 +4190,24 @@ func TestBitmapOrBulkMergeCopyOnWriteTailOwnership(t *testing.T) { t.Fatalf("source became invalid after tail mutations: %v", err) } } + +func TestValidateAcceptsRunContainersThatAreNotTheSmallestEncoding(t *testing.T) { + // AddOffset splits the run: the first container holds a run of two values, + // which takes more bytes than an array container would + bm := NewBitmap() + bm.AddRange(0, 10) + bm.RunOptimize() + shifted := AddOffset64(bm, 65534) + expected := NewBitmap() + expected.AddRange(65534, 65544) + assert.True(t, expected.Equals(shifted)) + assert.NoError(t, shifted.Validate()) + + serialized, err := shifted.ToBytes() + assert.NoError(t, err) + deserialized := NewBitmap() + _, err = deserialized.ReadFrom(bytes.NewReader(serialized)) + assert.NoError(t, err) + assert.NoError(t, deserialized.Validate()) + assert.True(t, expected.Equals(deserialized)) +} diff --git a/runcontainer.go b/runcontainer.go index 7c369b75..02d4a84f 100644 --- a/runcontainer.go +++ b/runcontainer.go @@ -65,9 +65,12 @@ var ( ErrRunNonSorted = errors.New("runs were not sorted") ErrRunIntervalEqual = errors.New("intervals were equal") ErrRunIntervalOverlap = errors.New("intervals overlapped or were continguous") - ErrRunIntervalSize = errors.New("too many intervals relative to data") - MaxNumIntervals = 2048 - MaxIntervalsSum = 2048 + // Deprecated: validate no longer requires a run container to be the smallest + // encoding of its values, so this error is not returned. + ErrRunIntervalSize = errors.New("too many intervals relative to data") + ErrRunIntervalOutOfRange = errors.New("interval extends past the end of the container") + MaxNumIntervals = 2048 + MaxIntervalsSum = 2048 ) func newInterval16Range(start, last uint16) interval16 { @@ -2947,15 +2950,19 @@ func (rc *runContainer16) validate() error { return ErrRunIntervalsEmpty } - intervalsSum := 0 + // check this first: interval16.last() wraps around past the end of the container + for _, iv := range rc.iv { + if int(iv.start)+int(iv.length) > int(MaxUint16) { + return ErrRunIntervalOutOfRange + } + } + for outeridx := range rc.iv { // The length being stored is the actual length - 1. // So we need to add 1 to get the actual length. // It is not possible to have a run with length 0. outerInterval := rc.iv[outeridx] - - intervalsSum += outerInterval.runlen() for inneridx := outeridx + 1; inneridx < len(rc.iv); inneridx++ { innerInterval := rc.iv[inneridx] @@ -2977,27 +2984,7 @@ func (rc *runContainer16) validate() error { } } - /* - if number of distinct values in the container >= 2048 then - check that the number of runs is no more than 2047 - (otherwise you could use a bitset container) - else - check that the number of runs < (number of distinct values) / 2 - (otherwise you could use an array container) - */ - - sizeAsRunContainer := runContainer16SerializedSizeInBytes(len(rc.iv)) - sizeAsBitmapContainer := bitmapContainerSizeInBytes() - sizeAsArrayContainer := arrayContainerSizeInBytes(intervalsSum) - // this is always ok: - if sizeAsRunContainer < minOfInt(sizeAsBitmapContainer, sizeAsArrayContainer) { - return nil - } - if sizeAsRunContainer >= sizeAsBitmapContainer { - return ErrRunIntervalSize - } - if sizeAsRunContainer >= sizeAsArrayContainer { - return ErrRunIntervalSize - } + // A run container need not be the smallest encoding of its values: operations such as + // AddOffset can produce run containers that runOptimize would convert. return nil } diff --git a/runcontainer_test.go b/runcontainer_test.go index 3dd9bcc6..f55f812d 100644 --- a/runcontainer_test.go +++ b/runcontainer_test.go @@ -2589,8 +2589,9 @@ func TestIntervalValidationFailing(t *testing.T) { rc.iv = append(rc.iv, a) } - assert.ErrorIs(t, rc.validate(), ErrRunIntervalSize) - // too many small runs, use array + // more runs than a bitmap container would need is still well formed + assert.NoError(t, rc.validate()) + // more runs than an array container would need is still well formed rc = &runContainer16{} start = -3 for i := 0; i < 10; i++ { @@ -2600,7 +2601,15 @@ func TestIntervalValidationFailing(t *testing.T) { rc.iv = append(rc.iv, a) } - assert.ErrorIs(t, rc.validate(), ErrRunIntervalSize) + assert.NoError(t, rc.validate()) + + // a run must end within the container + rc = &runContainer16{iv: []interval16{{start: 65000, length: 535}}} + assert.NoError(t, rc.validate()) + rc = &runContainer16{iv: []interval16{{start: 65000, length: 536}}} + assert.ErrorIs(t, rc.validate(), ErrRunIntervalOutOfRange) + rc = &runContainer16{iv: []interval16{{start: 10, length: 5}, {start: 65000, length: 2000}}} + assert.ErrorIs(t, rc.validate(), ErrRunIntervalOutOfRange) } func TestIntervalValidationsPassing(t *testing.T) { diff --git a/serialization_frozen_test.go b/serialization_frozen_test.go index ae00cc79..3ea43862 100644 --- a/serialization_frozen_test.go +++ b/serialization_frozen_test.go @@ -151,28 +151,34 @@ func TestBitMapValidationFromFrozen(t *testing.T) { err error }{ { - name: "Corrupts Run Length vs Num Runs", + name: "Last Run Extends Past Container End", loader: func(bm *Bitmap) { bm.AddRange(0, 2) bm.AddRange(4, 6) bm.AddRange(8, 100) + bm.RunOptimize() // one run container }, corruptor: func(s []byte) { - // 21 is the length of the run of the last run/range - // Shortening causes interval sum to be to short - s[10] = 1 + // 10-11 is the length of the last run, which starts at 8: + // 65535 makes it end past 65535 + s[10] = 0xFF + s[11] = 0xFF }, - err: ErrRunIntervalSize, + err: ErrRunIntervalOutOfRange, }, { - name: "Corrupts Run Length", + name: "Run Extends Past Container End", loader: func(bm *Bitmap) { bm.AddRange(100, 110) + bm.RunOptimize() // one run container }, corruptor: func(s []byte) { - s[2] = 0 + // 2-3 is the length of the run, which starts at 100: + // 65535 makes it end past 65535 + s[2] = 0xFF + s[3] = 0xFF }, - err: ErrRunIntervalSize, + err: ErrRunIntervalOutOfRange, }, { name: "Creates Interval Overlap", @@ -219,7 +225,7 @@ func TestBitMapValidationFromFrozen(t *testing.T) { if tt.err == nil { assert.NoError(t, frozenViewErr, "expected MustFrozenView to succeed when tt.err is nil") } else { - assert.Error(t, frozenViewErr, "expected MustFrozenView to fail when tt.err is not nil") + assert.ErrorIs(t, frozenViewErr, tt.err) } }) }