generalize test strings and break out old UnmarshalBinary code

(don't use iterator for unmarshalBinary)

ctl/check_test.go

@@ -93,7 +93,7 @@ func TestCheckCommand_Run(t *testing.T) {
 		t.Fatalf("copy: %v", err)
 	}
 
-	expectedPrefix := "checking bitmap: unmarshalling: unknown roaring magic number 12849"
+	expectedPrefix := "checking bitmap: unmarshalling: "
 	if !strings.HasPrefix(err.Error(), expectedPrefix) {
 		t.Fatalf("expect error: '%s...', actual: '%s'", expectedPrefix, err)
 	}

ctl/inspect_test.go

@@ -41,8 +41,8 @@ func TestInspectCommand_Run(t *testing.T) {
 	file.Close()
 	cm.Path = file.Name()
 	err = cm.Run(context.Background())
-	expectedError := "unmarshalling: unknown roaring magic number 12849"
-	if err != nil && err.Error() != expectedError {
+	expectedError := "unmarshalling: "
+	if !strings.Contains(err.Error(), expectedError) {
 		t.Fatalf("expected error '%s', got '%v'", expectedError, err)
 	}
 

roaring/fuzz_test.go

@@ -14,6 +14,7 @@
 package roaring
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -25,41 +26,41 @@ func TestUnmarshalBinary(t *testing.T) {
 	}{
 		{ // Checks for the zero containers situation
 			cr:       []byte(":0\x00\x00\x01\x00\x00\x000000"), //":000000"
-			expected: "reading official header: malformed bitmap, key-cardinality slice overruns buffer at 12",
+			expected: "header: malformed bitmap, key-cardinality slice overruns buffer at 12",
 		},
 		{ // The next 5 check for malformed bitmaps
 			cr: []byte("<0\x0000000000000000000" +
 				"\x00\x00\xec\x00\x03\x00\x00\x00\xec000"), //"<000000000000000000ÏÏ000"
-			expected: "insufficient data for header + offsets: want 12935430920 bytes, got 32",
+			expected: "insufficient data for header + offsets:",
 		},
 		{
 			cr: []byte("<0\x00\x02\x00\x00\x00\\f\x01\xb5\x8d\x009\v\x01\x00\x00\x00\x00" +
 				"\x00\x00e\x04\x00\x00\x00\x04\xfd\x00\x01\x00"), //"<0\f
µç9
e˝"
-			expected: "insufficient data for header + offsets: want 24696061960 bytes, got 32",
+			expected: "insufficient data for header + offsets:",
 		},
 		{
 			cr:       []byte("<0\x00\x02\x00\x00\x00&x.field safe"), //"<0&x.field safe"
-			expected: "insufficient data for header + offsets: want 10200547336 bytes, got 20",
+			expected: "insufficient data for header + offsets:",
 		},
 		{
 			cr: []byte("<0\x00\x00\x14\x00\x00\x00\x80\xffp\x05_ 4\x114089" +
 				"\x00\x00\xff\x000\x00\x02\x00\x00\x00\x00\xff\u007f\x00\x00\x01\x10\x00\x00j" +
 				"\x02\x00\x00$\x04_\x00\xff\u007f\xff062616163\x00" + //"<0ġp_ 44089ˇ0ˇj$_ˇˇ0626161630ø¸ad$j√"
 				"0\x00\x02\x00\x01\xbf\x00\x04\x00\xfcad$\x00\x00j\x10\x00\x00\xc3"),
-			expected: "insufficient data for header + offsets: want 328 bytes, got 80",
+			expected: "insufficient data for header + offsets:",
 		},
 		{ // 0 containers because the container is partially formed, but not fully (ie. 3/12 = 0)
 			cr:       []byte("<0\x00\x02\x03\x00\x00\x00쳫\v\x00d9\v\x00\x009\v"), //<0쳫d99
-			expected: "insufficient data for header + offsets: want 56 bytes, got 20",
+			expected: "insufficient data for header + offsets:",
 		},
 		{ // Checks for incomplete offset in readWithRuns
 			cr:       []byte(";0\x00\x00\v00000"), //";0000000"
-			expected: "container 0/1, expect run length at 9/10 bytes",
+			expected: "insufficient data for offsets",
 		},
 		{ // Checks for incomplete offset in readOffsets
 			cr: []byte(":0\x00\x00\x03\x00\x00\x00000000000000" +
 				"\x00"), //:0000000000000
-			expected: "insufficient data for offsets (need 12 bytes, found 1)",
+			expected: "insufficient data for offsets",
 		},
 	}
 
@@ -67,7 +68,7 @@ func TestUnmarshalBinary(t *testing.T) {
 		err := b.UnmarshalBinary(crash.cr)
 		if err == nil {
 			t.Errorf("expected: %s, got: no error", crash.expected)
-		} else if err.Error() != crash.expected {
+		} else if !strings.Contains(err.Error(), crash.expected) {
 			t.Errorf("expected: %s, got: %s", crash.expected, err)
 		}
 	}

roaring/roaring.go

@@ -1364,7 +1364,7 @@ func (r *officialRoaringIterator) Next() (key uint64, cType byte, n int, length
 	var runCount uint16
 	if r.currentType == containerRun {
 		if int(r.currentDataOffset)+2 > len(r.data) {
-			r.Done(fmt.Errorf("container %d/%d, expect run length at %d/%d bytes",
+			r.Done(fmt.Errorf("insufficient data for offsets container %d/%d, expect run length at %d/%d bytes",
 				r.currentIdx, r.keys, r.currentDataOffset, len(r.data)))
 			return r.Current()
 		}
@@ -1595,81 +1595,6 @@ func (b *Bitmap) PreferMapping(preferred bool) {
 	b.preferMapping = preferred
 }
 
-// unmarshalPilosaRoaring treats data as being encoded in Pilosa's 64 bit
-// roaring format and decodes it into b.
-func (b *Bitmap) UnmarshalBinary(data []byte) (err error) {
-	if data == nil {
-		return errors.New("no roaring bitmap provided")
-	}
-	var itr roaringIterator
-	var itrKey uint64
-	var itrCType byte
-	var itrN int
-	var itrLen int
-	var itrPointer *uint16
-	var itrErr error
-
-	itr, err = newRoaringIterator(data)
-	if err != nil {
-		return err
-	}
-	if itr == nil {
-		return errors.New("failed to create roaring iterator, but don't know why")
-	}
-
-	b.Containers.Reset()
-
-	itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next()
-	for itrErr == nil {
-		newC := &Container{
-			typeID:  itrCType,
-			n:       int32(itrN),
-			len:     int32(itrLen),
-			cap:     int32(itrLen),
-			pointer: itrPointer,
-			flags:   flagMapped,
-		}
-		if !b.preferMapping {
-			newC.unmapOrClone()
-		}
-		b.Containers.Put(itrKey, newC)
-		itrKey, itrCType, itrN, itrLen, itrPointer, itrErr = itr.Next()
-	}
-	// note: if we get a non-EOF err, it's possible that we made SOME
-	// changes but didn't log them. I don't have a good solution to this.
-	if itrErr != io.EOF {
-		return itrErr
-	}
-
-	// Read ops log until the end of the file.
-	b.ops = 0
-	b.opN = 0
-	buf := itr.Remaining()
-	for {
-		// Exit when there are no more ops to parse.
-		if len(buf) == 0 {
-			break
-		}
-
-		// Unmarshal the op and apply it.
-		var opr op
-		if err := opr.UnmarshalBinary(buf); err != nil {
-			// FIXME(benbjohnson): return error with position so file can be trimmed.
-			return err
-		}
-
-		opr.apply(b)
-
-		// Increase the op count.
-		b.ops++
-		b.opN += opr.count()
-
-		// Move the buffer forward.
-		buf = buf[opr.size():]
-	}
-	return nil
-}
-
 // writeOp writes op to the OpWriter, if available.
 func (b *Bitmap) writeOp(op *op) error {
 	if b.OpWriter == nil {

roaring/roaring_internal_test.go

@@ -3409,11 +3409,11 @@ func TestUnmarshalRoaringWithErrors(t *testing.T) {
 	}{
 		{ // Runs a bitmap without runs and no containers through the official roaring
 			hexString:     "3A30000000000000",
-			expectedError: "reading official header: malformed bitmap, key-cardinality slice overruns buffer at 8",
+			expectedError: "header: malformed bitmap, key-cardinality slice overruns buffer at 8",
 		},
 		{ // Runs a bitmap with runs and no containers through the official roaring
 			hexString:     "3B30000000000000",
-			expectedError: "reading official header: malformed bitmap, key-cardinality slice overruns buffer at 9",
+			expectedError: "header: malformed bitmap, key-cardinality slice overruns buffer at 9",
 		},
 		{ // Runs a bitmap in the Pilosa format through the Pilosa roaring
 			hexString: "3C30000000000000",
@@ -3427,7 +3427,7 @@ func TestUnmarshalRoaringWithErrors(t *testing.T) {
 		bm := NewBitmap()
 		err = bm.UnmarshalBinary(zeroContainers)
 		if err != nil {
-			if err.Error() != loopContainers.expectedError {
+			if !strings.Contains(err.Error(), loopContainers.expectedError) {
 				t.Fatalf("Expected: %s, Got: %s", loopContainers.expectedError, err)
 			}
 		}

roaring/roaring_unmarshal_binary.go

@@ -0,0 +1,208 @@
+// +build !enterprise
+
+package roaring
+
+import (
+	"encoding/binary"
+	"fmt"
+	"unsafe"
+
+	"github.com/pkg/errors"
+)
+
+// UnmarshalBinary decodes b from a binary-encoded byte slice. data can be in
+// either official roaring format or Pilosa's roaring format.
+func (b *Bitmap) UnmarshalBinary(data []byte) error {
+	if data == nil {
+		// Nothing to unmarshal
+		return nil
+	}
+	statsHit("Bitmap/UnmarshalBinary")
+	b.opN = 0 // reset opN since we're reading new data.
+	fileMagic := uint32(binary.LittleEndian.Uint16(data[0:2]))
+	if fileMagic == MagicNumber { // if pilosa roaring
+		return errors.Wrap(b.unmarshalPilosaRoaring(data), "unmarshaling as pilosa roaring")
+	}
+
+	keyN, containerTyper, header, pos, haveRuns, err := readOfficialHeader(data)
+	if err != nil {
+		return errors.Wrap(err, "reading roaring header")
+	}
+	// Only the Pilosa roaring format has flags. The official Roaring format
+	// hasn't got space in its header for flags.
+	b.Flags = 0
+
+	b.Containers.ResetN(int(keyN))
+	// Descriptive header section: Read container keys and cardinalities.
+	for i, buf := uint(0), data[header:]; i < uint(keyN); i, buf = i+1, buf[4:] {
+		card := int(binary.LittleEndian.Uint16(buf[2:4])) + 1
+		b.Containers.PutContainerValues(
+			uint64(binary.LittleEndian.Uint16(buf[0:2])),
+			containerTyper(i, card), /// container type voodo with isRunBitmap
+			card,
+			true)
+	}
+
+	// Read container offsets and attach data.
+	if haveRuns {
+		err := readWithRuns(b, data, pos, keyN)
+		if err != nil {
+			return errors.Wrap(err, "reading offsets from official roaring format")
+		}
+	} else {
+		err := readOffsets(b, data, pos, keyN)
+		if err != nil {
+			return errors.Wrap(err, "reading official roaring format")
+		}
+	}
+	return nil
+}
+
+func readOffsets(b *Bitmap, data []byte, pos int, keyN uint32) error {
+
+	citer, _ := b.Containers.Iterator(0)
+	for i, buf := 0, data[pos:]; i < int(keyN); i, buf = i+1, buf[4:] {
+		// Verify the offset is fully formed
+		if len(buf) < 4 {
+			return fmt.Errorf("insufficient data for offsets: len=%d", len(buf))
+		}
+		offset := binary.LittleEndian.Uint32(buf[0:4])
+		// Verify the offset is within the bounds of the input data.
+		if int(offset) >= len(data) {
+			return fmt.Errorf("offset out of bounds: off=%d, len=%d", offset, len(data))
+		}
+
+		// Map byte slice directly to the container data.
+		citer.Next()
+		_, c := citer.Value()
+		switch c.typ() {
+		case containerArray:
+			c.setArray((*[0xFFFFFFF]uint16)(unsafe.Pointer(&data[offset]))[:c.N():c.N()])
+		case containerBitmap:
+			c.setBitmap((*[0xFFFFFFF]uint64)(unsafe.Pointer(&data[offset]))[:bitmapN:bitmapN])
+		default:
+			return fmt.Errorf("unsupported container type %d", c.typ())
+		}
+	}
+	return nil
+}
+
+func readWithRuns(b *Bitmap, data []byte, pos int, keyN uint32) error {
+	if len(data) < pos+runCountHeaderSize {
+		return fmt.Errorf("insufficient data for offsets(run): len=%d", len(data))
+	}
+	citer, _ := b.Containers.Iterator(0)
+	for i := 0; i < int(keyN); i++ {
+		citer.Next()
+		_, c := citer.Value()
+		switch c.typ() {
+		case containerRun:
+			runCount := binary.LittleEndian.Uint16(data[pos : pos+runCountHeaderSize])
+			c.setRuns((*[0xFFFFFFF]interval16)(unsafe.Pointer(&data[pos+runCountHeaderSize]))[:runCount:runCount])
+			runs := c.runs()
+
+			for o := range runs { // must convert from start:length to start:end :(
+				runs[o].last = runs[o].start + runs[o].last
+			}
+			pos += int((runCount * interval16Size) + runCountHeaderSize)
+		case containerArray:
+			c.setArray((*[0xFFFFFFF]uint16)(unsafe.Pointer(&data[pos]))[:c.N():c.N()])
+			pos += int(c.N() * 2)
+		case containerBitmap:
+			c.setBitmap((*[0xFFFFFFF]uint64)(unsafe.Pointer(&data[pos]))[:bitmapN:bitmapN])
+			pos += bitmapN * 8
+		}
+	}
+	return nil
+}
+
+func (b *Bitmap) unmarshalPilosaRoaring(data []byte) error {
+	if len(data) < headerBaseSize {
+		return errors.New("data too small")
+	}
+
+	// Verify the first two bytes are a valid MagicNumber, and second two bytes match current storageVersion.
+	fileMagic := uint32(binary.LittleEndian.Uint16(data[0:2]))
+	fileVersion := uint32(data[2])
+	b.Flags = data[3]
+	if fileMagic != MagicNumber {
+		return fmt.Errorf("invalid roaring file, magic number %v is incorrect", fileMagic)
+	}
+
+	if fileVersion != storageVersion {
+		return fmt.Errorf("wrong roaring version, file is v%d, server requires v%d", fileVersion, storageVersion)
+	}
+
+	// Read key count in bytes sizeof(cookie)+sizeof(flag):(sizeof(cookie)+sizeof(uint32)).
+	keyN := binary.LittleEndian.Uint32(data[3+1 : 8])
+	if uint32(len(data)) < headerBaseSize+keyN*12 {
+		return fmt.Errorf("insufficient data for header + offsets: key-cardinality not provided for %d containers", int(keyN)/12)
+	}
+
+	headerSize := headerBaseSize
+	b.Containers.ResetN(int(keyN))
+	// Descriptive header section: Read container keys and cardinalities.
+	for i, buf := 0, data[headerSize:]; i < int(keyN); i, buf = i+1, buf[12:] {
+		b.Containers.PutContainerValues(
+			binary.LittleEndian.Uint64(buf[0:8]),
+			byte(binary.LittleEndian.Uint16(buf[8:10])),
+			int(binary.LittleEndian.Uint16(buf[10:12]))+1,
+			true)
+	}
+	opsOffset := headerSize + int(keyN)*12
+
+	// Read container offsets and attach data.
+	citer, _ := b.Containers.Iterator(0)
+	for i, buf := 0, data[opsOffset:]; i < int(keyN); i, buf = i+1, buf[4:] {
+		offset := binary.LittleEndian.Uint32(buf[0:4])
+		// Verify the offset is within the bounds of the input data.
+		if int(offset) >= len(data) {
+			return fmt.Errorf("offset out of bounds: off=%d, len=%d", offset, len(data))
+		}
+
+		// Map byte slice directly to the container data.
+		citer.Next()
+		_, c := citer.Value()
+
+		// this shouldn't happen, since we don't normally store nils.
+		if c == nil {
+			continue
+		}
+		switch c.typ() {
+		case containerRun:
+			runCount := binary.LittleEndian.Uint16(data[offset : offset+runCountHeaderSize])
+			c.setRuns((*[0xFFFFFFF]interval16)(unsafe.Pointer(&data[offset+runCountHeaderSize]))[:runCount:runCount])
+			opsOffset = int(offset) + runCountHeaderSize + len(c.runs())*interval16Size
+		case containerArray:
+			c.setArray((*[0xFFFFFFF]uint16)(unsafe.Pointer(&data[offset]))[:c.N():c.N()])
+			opsOffset = int(offset) + len(c.array())*2 // sizeof(uint32)
+		case containerBitmap:
+			c.setBitmap((*[0xFFFFFFF]uint64)(unsafe.Pointer(&data[offset]))[:bitmapN:bitmapN])
+			opsOffset = int(offset) + len(c.bitmap())*8 // sizeof(uint64)
+		}
+	}
+
+	// Read ops log until the end of the file.
+	buf := data[opsOffset:]
+
+	for {
+		// Exit when there are no more ops to parse.
+		if len(buf) == 0 {
+			break
+		}
+		// Unmarshal the op and apply it.
+		var opr op
+		if err := opr.UnmarshalBinary(buf); err != nil {
+			// FIXME(benbjohnson): return error with position so file can be trimmed.
+			return err
+		}
+		opr.apply(b)
+		// Increase the op count.
+		b.ops++
+		b.opN += opr.count()
+		// Move the buffer forward.
+		buf = buf[opr.size():]
+	}
+
+	return nil
+}