Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example test for Frozen Bitmap fault when using Frozen Bitmaps as regular/mutable bitmaps with a read-only buffer #362

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ require (
github.com/bits-and-blooms/bitset v1.2.0
github.com/mschoch/smat v0.2.0
github.com/stretchr/testify v1.4.0
golang.org/x/sys v0.0.0-20220622161953-175b2fd9d664
)
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZN
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
golang.org/x/sys v0.0.0-20220622161953-175b2fd9d664 h1:wEZYwx+kK+KlZ0hpvP2Ls1Xr4+RWnlzGFwPP0aiDjIU=
golang.org/x/sys v0.0.0-20220622161953-175b2fd9d664/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
Expand Down
92 changes: 92 additions & 0 deletions serialization_frozen_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@ package roaring

import (
"bytes"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"golang.org/x/sys/unix"
"io/ioutil"
"os"
"reflect"
"runtime/debug"
"testing"
)

Expand Down Expand Up @@ -126,3 +131,90 @@ func TestFrozenFormat(t *testing.T) {
})
}
}

type frozenTestCase struct {
name string
useMMapped bool
useFrozen bool
shouldPanic bool
}

func TestFrozenCases(t *testing.T) {
cases := []frozenTestCase{
{name: "in-memory-frozen", useMMapped: false,
useFrozen: true,
shouldPanic: false},
{name: "mmap-frozen", useMMapped: true,
useFrozen: true,
// THIS SHOULD NOT BE PANIC/FAULTING
shouldPanic: true},
{name: "in-memory", useMMapped: false,
useFrozen: false,
shouldPanic: false},
{name: "mmap", useMMapped: true,
useFrozen: false,
shouldPanic: false},
}
for _, testCase := range cases {
testFrozenCase(t, testCase)
}
}
func testFrozenCase(t *testing.T, testCase frozenTestCase) {

startingBitmap := NewBitmap()
startingBitmap.highlowcontainer.appendContainer(0, &runContainer16{iv: []interval16{{0, 0},
{2, 5}}}, false)

primary := getBitmap(t, testCase, startingBitmap)
assert.True(t, startingBitmap.Equals(primary))
clone := primary.Clone()
primary.Xor(clone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are modifying, in-place, the frozen view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider doing

I would do

newprimary := Xor(primary, clone)

res := primary.GetCardinality()
assert.Equal(t, res, uint64(0))
if testCase.shouldPanic {
assert.Panics(t, func() {
debug.SetPanicOnFault(true)
primary.Xor(clone)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are, again, modifying the frozen view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would do

newprimary := Xor(primary, clone)

I am not sure why you want to mutate the frozen view.

})
} else {
primary.Xor(clone)
}
}

func getBitmap(t *testing.T, testCase frozenTestCase, startingBitmap *Bitmap) *Bitmap {
receiver := NewBitmap()
if testCase.useFrozen {
frozenBytes, err := startingBitmap.Freeze()
require.NoError(t, err)
if testCase.useMMapped {
require.NoError(t, receiver.FrozenView(getBytesAsMMap(t, frozenBytes)))
return receiver
}
require.NoError(t, receiver.FrozenView(frozenBytes))
return receiver
}
nonFrozenBytes, err := startingBitmap.ToBytes()
require.NoError(t, err)

if testCase.useMMapped {
_, err = receiver.FromBuffer(getBytesAsMMap(t, nonFrozenBytes))
require.NoError(t, err)
return receiver
}
_, err = receiver.FromBuffer(nonFrozenBytes)
require.NoError(t, err)
return receiver
}

func getBytesAsMMap(t *testing.T, startingBytes []byte) []byte {
os.WriteFile("data", startingBytes, 0777)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely, there is some needed error handling here?

Note that os.WriteFile is go 1.16. I think you want ioutil.WriteFile for portability.

file, err := os.OpenFile("data", os.O_RDONLY, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file is read-only.

require.NoError(t, err)
fileinfo, err := file.Stat()
require.NoError(t, err)

bytes, err := unix.Mmap(int(file.Fd()), 0, int(fileinfo.Size()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a ressource leak around it, surely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytes has to be immutable, it cannot be written to.

unix.PROT_READ, unix.MAP_PRIVATE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PROT_READ means that the data can be read, it cannot be written to.

require.NoError(t, err)
return bytes
}