From 3ef2c84d3c5d2664dee8da96bd995b2a0bedb2fa Mon Sep 17 00:00:00 2001
From: Seebs <seebs@seebs.net>
Date: Mon, 29 Mar 2021 12:29:32 -0500
Subject: [PATCH] bitmap unmarshalling and testing bug fixes

When unmarshalling ops, we weren't adding a meaningful OpN to them,
resulting in misleading reports from `pilosa inspect`. Also, we were
mistakenly reporting things as "mapped" when they were actually
using their internal storage (as with small array containers).

Add the "sanity check" to `pilosa inspect` so that errors like the
above get noticed more easily and corrected. Also, to make that work,
have roaring.InspectBinary actually put containers in the bitmap
it creates rather than just creating info entries for them.
---
 ctl/inspect.go              | 11 ++++++++++-
 roaring/roaring.go          |  7 ++++---
 roaring/unmarshal_binary.go | 12 ++++++++++--
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/ctl/inspect.go b/ctl/inspect.go
index 09de20370..55a3eaf35 100644
--- a/ctl/inspect.go
+++ b/ctl/inspect.go
@@ -29,6 +29,7 @@ import (
 	"syscall"
 	"text/tabwriter"
 	"time"
+	"unsafe"
 
 	"github.com/gogo/protobuf/proto"
 	"github.com/pilosa/pilosa/v2"
@@ -360,16 +361,24 @@ func (cmd *InspectCommand) InspectFile(f *os.File, fi os.FileInfo) error {
 			fmt.Fprintf(cmd.Stderr, "inspect command: munmap failed: %v", err)
 		}
 	}()
+	mappedFrom := uintptr(unsafe.Pointer(&data[0]))
+	mappedTo := mappedFrom + uintptr(len(data))
 	// Attach the mmap file to the bitmap.
 	t := time.Now()
 	fmt.Fprintf(cmd.Stderr, "inspecting bitmap...")
 	var info roaring.BitmapInfo
-	_, _, err = roaring.InspectBinary(data, true, &info)
+	bitmap, _, err := roaring.InspectBinary(data, true, &info)
 	fmt.Fprintf(cmd.Stderr, " (%s)\n", time.Since(t))
 	cmd.DisplayInfo(info)
 	if err != nil {
 		return errors.Wrap(err, "inspecting")
 	}
+	mappedIn, mappedOut, unmappedIn, errs, err := bitmap.SanityCheckMapping(mappedFrom, mappedTo)
+	if err != nil {
+		fmt.Fprintf(cmd.Stderr, "sanity check: %d mapped in, %d mapped out, %d unmapped in, %d errors\n",
+			mappedIn, mappedOut, unmappedIn, errs)
+		fmt.Fprintf(cmd.Stderr, "last error: %v\n", err)
+	}
 	return nil
 }
 
diff --git a/roaring/roaring.go b/roaring/roaring.go
index d5ec88712..5f7c8e97b 100644
--- a/roaring/roaring.go
+++ b/roaring/roaring.go
@@ -5957,7 +5957,7 @@ func (op *op) UnmarshalBinary(data []byte) error {
 
 	switch op.typ {
 	case opTypeAdd, opTypeRemove:
-		// nothing to do, just being not-default
+		op.opN = 1
 	case opTypeAddBatch, opTypeRemoveBatch:
 		// This ensures that in doing 13+op.value*8, the max int won't be exceeded and a wrap around case
 		// (resulting in a negative value) won't occur in the slice indexing while writing
@@ -5968,8 +5968,9 @@ func (op *op) UnmarshalBinary(data []byte) error {
 			return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data))
 		}
 		_, _ = h.Write(data[13 : 13+op.value*8])
-		op.values = make([]uint64, op.value)
-		for i := uint64(0); i < op.value; i++ {
+		op.opN = int(op.value)
+		op.values = make([]uint64, op.opN)
+		for i := range op.values {
 			start := 13 + i*8
 			op.values[i] = binary.LittleEndian.Uint64(data[start : start+8])
 		}
diff --git a/roaring/unmarshal_binary.go b/roaring/unmarshal_binary.go
index 72d73e961..a29bba516 100644
--- a/roaring/unmarshal_binary.go
+++ b/roaring/unmarshal_binary.go
@@ -58,7 +58,10 @@ func (b *Bitmap) UnmarshalBinary(data []byte) (err error) {
 		default:
 			panic("invalid container type")
 		}
-		newC.setMapped(true)
+		// If we're using the iterator's pointer, we're "mapped". But
+		// for instance, small arrays may use their own data structures,
+		// which is fine.
+		newC.setMapped(newC.pointer == itrPointer)
 		if !b.preferMapping {
 			newC = newC.unmapOrClone()
 		}
@@ -150,10 +153,14 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe
 		default:
 			panic("invalid container type")
 		}
-		newC.setMapped(true)
+		// If our pointer isn't itrPointer, we aren't actually mapped.
+		newC.setMapped(newC.pointer == itrPointer)
 		if !mapped {
 			newC = newC.unmapOrClone()
 		}
+		// Pristine means this is the original object read in from
+		// roaring data, even if it's not mapped, which this is for
+		// now.
 		newC.flags |= flagPristine
 		if newC.flags&flagMapped != 0 {
 			mappedAny = true
@@ -169,6 +176,7 @@ func InspectBinary(data []byte, mapped bool, info *BitmapInfo) (b *Bitmap, mappe
 		})
 		info.ContainerCount++
 		info.BitCount += uint64(newC.n)
+		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