Validate: reject runs past the container end, accept non-minimal run containers

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

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

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
 }

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

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