From 68e91bf352e567fec43ca73dadcaeb2a37fdd322 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 20 Dec 2018 18:23:10 -0800 Subject: [PATCH 1/3] fix a potential overflow bug If we have a deep enough directory, we could walk off the end of the bit array. --- hamt/hamt.go | 10 ++++++++-- hamt/util.go | 14 +++++++++++--- hamt/util_test.go | 35 +++++++++++++++++++++++++++-------- 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/hamt/hamt.go b/hamt/hamt.go index 3714c30a2..8108da3d3 100644 --- a/hamt/hamt.go +++ b/hamt/hamt.go @@ -375,7 +375,10 @@ func (ds *Shard) rmChild(i int) error { } func (ds *Shard) getValue(ctx context.Context, hv *hashBits, key string, cb func(*Shard) error) error { - idx := hv.Next(ds.tableSizeLg2) + idx, err := hv.Next(ds.tableSizeLg2) + if err != nil { + return fmt.Errorf("sharded directory too deep") + } if ds.bitfield.Bit(int(idx)) { cindex := ds.indexForBitPos(idx) @@ -516,7 +519,10 @@ func (ds *Shard) walkTrie(ctx context.Context, cb func(*Shard) error) error { } func (ds *Shard) modifyValue(ctx context.Context, hv *hashBits, key string, val *ipld.Link) error { - idx := hv.Next(ds.tableSizeLg2) + idx, err := hv.Next(ds.tableSizeLg2) + if err != nil { + return fmt.Errorf("sharded directory too deep") + } if !ds.bitfield.Bit(idx) { return ds.insertChild(idx, key, val) } diff --git a/hamt/util.go b/hamt/util.go index 5f684a21a..927d44964 100644 --- a/hamt/util.go +++ b/hamt/util.go @@ -15,8 +15,16 @@ func mkmask(n int) byte { return (1 << uint(n)) - 1 } -// Next returns the next 'i' bits of the hashBits value as an integer -func (hb *hashBits) Next(i int) int { +// Next returns the next 'i' bits of the hashBits value as an integer, or an +// error if there aren't enough bits. +func (hb *hashBits) Next(i int) (int, error) { + if hb.consumed+i > len(hb.b)*8 { + return 0, fmt.Errorf("not enough bits remaining") + } + return hb.next(i), nil +} + +func (hb *hashBits) next(i int) int { curbi := hb.consumed / 8 leftb := 8 - (hb.consumed % 8) @@ -35,7 +43,7 @@ func (hb *hashBits) Next(i int) int { out := int(mkmask(leftb) & curb) out <<= uint(i - leftb) hb.consumed += leftb - out += hb.Next(i - leftb) + out += hb.next(i - leftb) return out } } diff --git a/hamt/util_test.go b/hamt/util_test.go index b1cbc5217..835c17428 100644 --- a/hamt/util_test.go +++ b/hamt/util_test.go @@ -9,37 +9,56 @@ func TestHashBitsEvenSizes(t *testing.T) { hb := hashBits{b: buf} for _, v := range buf { - if hb.Next(8) != int(v) { - t.Fatal("got wrong numbers back") + if a, _ := hb.Next(8); a != int(v) { + t.Fatalf("got wrong numbers back: expected %d, got %d", v, a) } } } +func TestHashBitsOverflow(t *testing.T) { + buf := []byte{255} + hb := hashBits{b: buf} + + for i := 0; i < 8; i++ { + bit, err := hb.Next(1) + if err != nil { + t.Fatalf("got %d bits back, expected 8: %s", i, err) + } + if bit != 1 { + t.Fatal("expected all one bits") + } + } + _, err := hb.Next(1) + if err == nil { + t.Error("overflowed the bit vector") + } +} + func TestHashBitsUneven(t *testing.T) { buf := []byte{255, 127, 79, 45, 116, 99, 35, 17} hb := hashBits{b: buf} - v := hb.Next(4) + v, _ := hb.Next(4) if v != 15 { t.Fatal("should have gotten 15: ", v) } - v = hb.Next(4) + v, _ = hb.Next(4) if v != 15 { t.Fatal("should have gotten 15: ", v) } - if v := hb.Next(3); v != 3 { + if v, _ := hb.Next(3); v != 3 { t.Fatalf("expected 3, but got %b", v) } - if v := hb.Next(3); v != 7 { + if v, _ := hb.Next(3); v != 7 { t.Fatalf("expected 7, but got %b", v) } - if v := hb.Next(3); v != 6 { + if v, _ := hb.Next(3); v != 6 { t.Fatalf("expected 6, but got %b", v) } - if v := hb.Next(15); v != 20269 { + if v, _ := hb.Next(15); v != 20269 { t.Fatalf("expected 20269, but got %b (%d)", v, v) } } From 6e4a0b54ebf90cd60ef35971dc2eb568db192536 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 20 Dec 2018 18:26:59 -0800 Subject: [PATCH 2/3] keep the full murmur128 hash 1. It's less confusing. Murmur64 is something our library supports by truncating a Murmur128 hash. 2. We'll only use the part we need anyways. directory trees. --- hamt/hamt.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hamt/hamt.go b/hamt/hamt.go index 8108da3d3..e59f88173 100644 --- a/hamt/hamt.go +++ b/hamt/hamt.go @@ -203,9 +203,9 @@ func (ds *Shard) makeShardValue(lnk *ipld.Link) (*Shard, error) { } func hash(val []byte) []byte { - h := murmur3.New64() + h := murmur3.New128() h.Write(val) - return h.Sum(nil) + return h.Sum(make([]byte, 0, 128/8)) } // Set sets 'name' = nd in the HAMT From f3b122d2a5e64be72d9b9db8d0c63fe82b022c18 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 20 Dec 2018 19:23:56 -0800 Subject: [PATCH 3/3] return the sharded directory error from the hash bits helper I didn't do this before because this datastructure *technically* isn't specific to sharded directories but, really, that was just even more confusing. --- hamt/hamt.go | 4 ++-- hamt/util.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hamt/hamt.go b/hamt/hamt.go index e59f88173..fdaf5be9b 100644 --- a/hamt/hamt.go +++ b/hamt/hamt.go @@ -377,7 +377,7 @@ func (ds *Shard) rmChild(i int) error { func (ds *Shard) getValue(ctx context.Context, hv *hashBits, key string, cb func(*Shard) error) error { idx, err := hv.Next(ds.tableSizeLg2) if err != nil { - return fmt.Errorf("sharded directory too deep") + return err } if ds.bitfield.Bit(int(idx)) { cindex := ds.indexForBitPos(idx) @@ -521,7 +521,7 @@ func (ds *Shard) walkTrie(ctx context.Context, cb func(*Shard) error) error { func (ds *Shard) modifyValue(ctx context.Context, hv *hashBits, key string, val *ipld.Link) error { idx, err := hv.Next(ds.tableSizeLg2) if err != nil { - return fmt.Errorf("sharded directory too deep") + return err } if !ds.bitfield.Bit(idx) { return ds.insertChild(idx, key, val) diff --git a/hamt/util.go b/hamt/util.go index 927d44964..c2b33bc22 100644 --- a/hamt/util.go +++ b/hamt/util.go @@ -19,7 +19,7 @@ func mkmask(n int) byte { // error if there aren't enough bits. func (hb *hashBits) Next(i int) (int, error) { if hb.consumed+i > len(hb.b)*8 { - return 0, fmt.Errorf("not enough bits remaining") + return 0, fmt.Errorf("sharded directory too deep") } return hb.next(i), nil }