From a1f6321b1dfbdc0d95e6eb9bfc1adae87b9ebac2 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Thu, 13 Jun 2019 11:54:52 -0500 Subject: [PATCH 01/12] Added a test for slice bounds out of range --- roaring/fuzz_test.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 roaring/fuzz_test.go diff --git a/roaring/fuzz_test.go b/roaring/fuzz_test.go new file mode 100644 index 000000000..c951b412e --- /dev/null +++ b/roaring/fuzz_test.go @@ -0,0 +1,23 @@ +package roaring + +import ( + "testing" +) + +func TestUnmarshalBinary(t *testing.T) { + b := NewBitmap() + confirmedCrashers := []struct { + cr []byte + } { + {cr : []byte(":000000")}, + {cr : []byte("<000000000000000")}, + } + + for _, crash := range confirmedCrashers { + err := b.UnmarshalBinary(crash.cr) + if err != nil { + t.Error("Known crasher failed.") + } + } + +} \ No newline at end of file From 0a87d8108f82a865ebcd91e4278661d05a853b8a Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Fri, 14 Jun 2019 10:05:06 -0500 Subject: [PATCH 02/12] Added the actual bytes and their respective errors --- roaring/fuzz_test.go | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/roaring/fuzz_test.go b/roaring/fuzz_test.go index c951b412e..50a1492a2 100644 --- a/roaring/fuzz_test.go +++ b/roaring/fuzz_test.go @@ -8,16 +8,24 @@ func TestUnmarshalBinary(t *testing.T) { b := NewBitmap() confirmedCrashers := []struct { cr []byte + expected string } { - {cr : []byte(":000000")}, - {cr : []byte("<000000000000000")}, + { + cr : []byte(":0\x000\x01\x00\x00\x000000"), //":000000" + expected : "reading roaring header: malformed bitmap, key-cardinality slice overruns buffer at 12", + }, + { + cr : []byte("<0\x000\x00\x00\x00\x00000000000000" + + "0"), //"<000000000000000" + expected : "unmarshaling as pilosa roaring: too big", + }, } for _, crash := range confirmedCrashers { err := b.UnmarshalBinary(crash.cr) - if err != nil { - t.Error("Known crasher failed.") - } + if err.Error() != crash.expected { + t.Errorf("Expected: %s, Got: %s", crash.expected, err) + } } } \ No newline at end of file From 622fba4f2752503f62f2b4d90ca8837c425c19bf Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Fri, 14 Jun 2019 10:06:37 -0500 Subject: [PATCH 03/12] Fixed the <000000000 bug by adding if statement --- roaring/roaring.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/roaring/roaring.go b/roaring/roaring.go index 8930981e0..5437bba83 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3963,6 +3963,9 @@ func (op *op) UnmarshalBinary(data []byte) error { _, _ = h.Write(data[0:9]) if op.typ > 1 { + if 1152921504606847000 < int(op.value) { + return fmt.Errorf("too big") + } if len(data) < int(13+op.value*8) { return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data)) } From 1e7638677b117269292155384911886f23959f78 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Fri, 14 Jun 2019 10:08:23 -0500 Subject: [PATCH 04/12] Fixed the :000000 bug by adding an = in readOfficalHeader --- roaring/roaring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 5437bba83..023c75bc4 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -4458,7 +4458,7 @@ func readOfficialHeader(buf []byte) (size uint32, containerTyper func(index uint } // descriptive header - if pos+2*2*int(size) > len(buf) { + if pos+2*2*int(size) >= len(buf) { err = fmt.Errorf("malformed bitmap, key-cardinality slice overruns buffer at %d", pos+2*2*int(size)) return size, containerTyper, header, pos, flags, haveRuns, err } From 56659f9d7b36f6d89b27dd87786e10f73b501165 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Fri, 14 Jun 2019 11:47:33 -0500 Subject: [PATCH 05/12] Added Licensing --- roaring/fuzz_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/roaring/fuzz_test.go b/roaring/fuzz_test.go index 50a1492a2..b11d00337 100644 --- a/roaring/fuzz_test.go +++ b/roaring/fuzz_test.go @@ -1,3 +1,16 @@ +// Copyright 2017 Pilosa Corp. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. package roaring import ( From 77cb1ea6d8a3e1f8d54dbcf2690034bf3aa83a83 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Fri, 14 Jun 2019 13:44:04 -0500 Subject: [PATCH 06/12] Claified the arithmetic behind the max op.value --- roaring/roaring.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 31916445f..f1593df9c 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3961,7 +3961,10 @@ func (op *op) UnmarshalBinary(data []byte) error { _, _ = h.Write(data[0:9]) if op.typ > 1 { - if 1152921504606847000 < int(op.value) { + // The maximum integer value for a int64 is 9223372036854775807 + maxInt := 9223372036854775807 + maxOpValue := maxInt/8-13 + if maxOpValue < int(op.value) { return fmt.Errorf("too big") } if len(data) < int(13+op.value*8) { From 734daf79ee99d9486b43602c9d398c3851572aaa Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Fri, 14 Jun 2019 14:30:21 -0500 Subject: [PATCH 07/12] Simplified the if statement and made the calculation more precise --- roaring/roaring.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index f1593df9c..118a2f0ac 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -23,6 +23,7 @@ import ( "math/bits" "sort" "unsafe" + "math" "github.com/pkg/errors" ) @@ -3956,17 +3957,15 @@ func (op *op) UnmarshalBinary(data []byte) error { // op.value will actually contain the length of values for batch ops op.value = binary.LittleEndian.Uint64(data[1:9]) + if math.MaxInt64/8-13 < int(op.value){ + return fmt.Errorf("too big") + } + // Verify checksum. h := fnv.New32a() _, _ = h.Write(data[0:9]) if op.typ > 1 { - // The maximum integer value for a int64 is 9223372036854775807 - maxInt := 9223372036854775807 - maxOpValue := maxInt/8-13 - if maxOpValue < int(op.value) { - return fmt.Errorf("too big") - } if len(data) < int(13+op.value*8) { return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data)) } From 413492552c0e13333d56b17186de9f50be19b69f Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Mon, 17 Jun 2019 08:36:56 -0500 Subject: [PATCH 08/12] Rearranged if statement and declared maxOpSize value --- roaring/roaring.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 118a2f0ac..218227a61 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3945,6 +3945,7 @@ func (op *op) WriteTo(w io.Writer) (n int64, err error) { } var minOpSize = 13 +var maxOpSize = math.MaxInt64/8 - 13 // UnmarshalBinary decodes data into an op. func (op *op) UnmarshalBinary(data []byte) error { @@ -3957,15 +3958,14 @@ func (op *op) UnmarshalBinary(data []byte) error { // op.value will actually contain the length of values for batch ops op.value = binary.LittleEndian.Uint64(data[1:9]) - if math.MaxInt64/8-13 < int(op.value){ - return fmt.Errorf("too big") - } - // Verify checksum. h := fnv.New32a() _, _ = h.Write(data[0:9]) if op.typ > 1 { + if maxOpSize < int(op.value){ + return fmt.Errorf("too big") + } if len(data) < int(13+op.value*8) { return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data)) } From d9f2792d1fab73face48fbc7fcd10d0eac78c348 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Mon, 17 Jun 2019 14:11:37 -0500 Subject: [PATCH 09/12] Reworded max int error and reset max int value --- roaring/roaring.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index 218227a61..db2910a8e 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -23,7 +23,6 @@ import ( "math/bits" "sort" "unsafe" - "math" "github.com/pkg/errors" ) @@ -3945,7 +3944,7 @@ func (op *op) WriteTo(w io.Writer) (n int64, err error) { } var minOpSize = 13 -var maxOpSize = math.MaxInt64/8 - 13 +var maxOpN = 1000000 // UnmarshalBinary decodes data into an op. func (op *op) UnmarshalBinary(data []byte) error { @@ -3963,8 +3962,8 @@ func (op *op) UnmarshalBinary(data []byte) error { _, _ = h.Write(data[0:9]) if op.typ > 1 { - if maxOpSize < int(op.value){ - return fmt.Errorf("too big") + if maxOpN < int(op.value){ + return fmt.Errorf("Maximum operation size exceeded") } if len(data) < int(13+op.value*8) { return fmt.Errorf("op data truncated - expected %d, got %d", 13+op.value*8, len(data)) From d24a15794775413e164e86e0ef0307a55fb6f78d Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Mon, 17 Jun 2019 15:58:20 -0500 Subject: [PATCH 10/12] Addressed review feedback --- roaring/fuzz_test.go | 2 +- roaring/roaring.go | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/roaring/fuzz_test.go b/roaring/fuzz_test.go index b11d00337..69362d947 100644 --- a/roaring/fuzz_test.go +++ b/roaring/fuzz_test.go @@ -30,7 +30,7 @@ func TestUnmarshalBinary(t *testing.T) { { cr : []byte("<0\x000\x00\x00\x00\x00000000000000" + "0"), //"<000000000000000" - expected : "unmarshaling as pilosa roaring: too big", + expected : "unmarshaling as pilosa roaring: Maximum operation size exceeded", }, } diff --git a/roaring/roaring.go b/roaring/roaring.go index db2910a8e..e10793915 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3944,7 +3944,7 @@ func (op *op) WriteTo(w io.Writer) (n int64, err error) { } var minOpSize = 13 -var maxOpN = 1000000 +var maxBatchSize = 1<<59 // UnmarshalBinary decodes data into an op. func (op *op) UnmarshalBinary(data []byte) error { @@ -3962,7 +3962,9 @@ func (op *op) UnmarshalBinary(data []byte) error { _, _ = h.Write(data[0:9]) if op.typ > 1 { - if maxOpN < int(op.value){ + // 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 + if int(op.value) > maxBatchSize { return fmt.Errorf("Maximum operation size exceeded") } if len(data) < int(13+op.value*8) { From 2d151cd41a434ed7780165c54c2945001f0ead81 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Tue, 18 Jun 2019 16:48:03 -0500 Subject: [PATCH 11/12] Making CI happy --- roaring/roaring.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index e10793915..cb1a7cb9b 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3944,7 +3944,7 @@ func (op *op) WriteTo(w io.Writer) (n int64, err error) { } var minOpSize = 13 -var maxBatchSize = 1<<59 +var maxBatchSize = uint64(1<<59) // UnmarshalBinary decodes data into an op. func (op *op) UnmarshalBinary(data []byte) error { @@ -3964,7 +3964,7 @@ func (op *op) UnmarshalBinary(data []byte) error { if op.typ > 1 { // 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 - if int(op.value) > maxBatchSize { + if int(op.value) > int(maxBatchSize) { return fmt.Errorf("Maximum operation size exceeded") } if len(data) < int(13+op.value*8) { From 3eed3b472fc6a3fae895374cadbc2ef52d169697 Mon Sep 17 00:00:00 2001 From: Ashley Svetlik Date: Wed, 19 Jun 2019 10:36:04 -0500 Subject: [PATCH 12/12] Corrected If statement logic error --- roaring/roaring.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roaring/roaring.go b/roaring/roaring.go index cb1a7cb9b..d7334be05 100644 --- a/roaring/roaring.go +++ b/roaring/roaring.go @@ -3964,7 +3964,7 @@ func (op *op) UnmarshalBinary(data []byte) error { if op.typ > 1 { // 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 - if int(op.value) > int(maxBatchSize) { + if op.value > maxBatchSize { return fmt.Errorf("Maximum operation size exceeded") } if len(data) < int(13+op.value*8) {