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) } }) }