From ed0a5c73cdac6465607c7374a55fc841132bd128 Mon Sep 17 00:00:00 2001 From: Lucas Molas Date: Mon, 26 Jun 2023 15:57:31 +0200 Subject: [PATCH 1/4] ipld/unixfs/hamt: catch panic in walkChildren --- ipld/unixfs/hamt/hamt.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/ipld/unixfs/hamt/hamt.go b/ipld/unixfs/hamt/hamt.go index fc6cda797..c297fb780 100644 --- a/ipld/unixfs/hamt/hamt.go +++ b/ipld/unixfs/hamt/hamt.go @@ -28,8 +28,6 @@ import ( "os" "sync" - "golang.org/x/sync/errgroup" - format "github.com/ipfs/boxo/ipld/unixfs" "github.com/ipfs/boxo/ipld/unixfs/internal" @@ -37,8 +35,12 @@ import ( bitfield "github.com/ipfs/go-bitfield" cid "github.com/ipfs/go-cid" ipld "github.com/ipfs/go-ipld-format" + logging "github.com/ipfs/go-log/v2" + "golang.org/x/sync/errgroup" ) +var log = logging.Logger("unixfs-hamt") + const ( // HashMurmur3 is the multiformats identifier for Murmur3 HashMurmur3 uint64 = 0x22 @@ -429,8 +431,13 @@ type listCidsAndShards struct { func (ds *Shard) walkChildren(processLinkValues func(formattedLink *ipld.Link) error) (*listCidsAndShards, error) { res := &listCidsAndShards{} - for idx, lnk := range ds.childer.links { - if nextShard := ds.childer.children[idx]; nextShard == nil { + for i := range ds.childer.children { + if nextShard := ds.childer.child(i); nextShard == nil { + lnk := ds.childer.link(i) + if lnk == nil { + log.Warnf("internal HAMT error: both link and shard nil at pos %d, dumping shard: %+v", i, *ds) + return nil, fmt.Errorf("internal HAMT error: both link and shard nil, check log") + } lnkLinkType, err := ds.childLinkType(lnk) if err != nil { return nil, err From 7b7329e1c29e36fe8e62c939bb167d551a57739f Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:23:19 -0700 Subject: [PATCH 2/4] Add test for nil link and shard --- ipld/unixfs/hamt/hamt.go | 5 ++--- ipld/unixfs/hamt/hamt_test.go | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/ipld/unixfs/hamt/hamt.go b/ipld/unixfs/hamt/hamt.go index 3eafddd21..455d070c6 100644 --- a/ipld/unixfs/hamt/hamt.go +++ b/ipld/unixfs/hamt/hamt.go @@ -432,8 +432,8 @@ type listCidsAndShards struct { func (ds *Shard) walkChildren(processLinkValues func(formattedLink *ipld.Link) error) (*listCidsAndShards, error) { res := &listCidsAndShards{} - for i := range ds.childer.children { - if nextShard := ds.childer.child(i); nextShard == nil { + for i, nextShard := range ds.childer.children { + if nextShard == nil { lnk := ds.childer.link(i) if lnk == nil { log.Warnf("internal HAMT error: both link and shard nil at pos %d, dumping shard: %+v", i, *ds) @@ -461,7 +461,6 @@ func (ds *Shard) walkChildren(processLinkValues func(formattedLink *ipld.Link) e default: return nil, errors.New("unsupported shard link type") } - } else { if nextShard.val != nil { formattedLink := &ipld.Link{ diff --git a/ipld/unixfs/hamt/hamt_test.go b/ipld/unixfs/hamt/hamt_test.go index 1946bbd7c..72c08fa5e 100644 --- a/ipld/unixfs/hamt/hamt_test.go +++ b/ipld/unixfs/hamt/hamt_test.go @@ -749,3 +749,24 @@ func TestHamtBadSize(t *testing.T) { } } } + +func TestHamtNilChildren(t *testing.T) { + shard, err := NewShard(nil, 1024) + if err != nil { + t.Fatal(err) + } + shard.childer = shard.childer.makeChilder(nil, []*ipld.Link{nil}) + nextShard, err := shard.walkChildren(func(_ *ipld.Link) error { + t.Fatal("processLinkValues function should not have been called") + return nil + }) + if err == nil { + t.Fatal("expected error") + } + if err.Error() != "internal HAMT error: both link and shard nil, check log" { + t.Fatal("did not get expected error") + } + if nextShard != nil { + t.Fatal("nextShard should be nil") + } +} From 13e720333bd924b968f7e1cb85957a0136b0bac6 Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:24:38 -0700 Subject: [PATCH 3/4] rename test --- ipld/unixfs/hamt/hamt_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipld/unixfs/hamt/hamt_test.go b/ipld/unixfs/hamt/hamt_test.go index 72c08fa5e..16b325773 100644 --- a/ipld/unixfs/hamt/hamt_test.go +++ b/ipld/unixfs/hamt/hamt_test.go @@ -750,7 +750,7 @@ func TestHamtBadSize(t *testing.T) { } } -func TestHamtNilChildren(t *testing.T) { +func TestHamtNilLinkAndShard(t *testing.T) { shard, err := NewShard(nil, 1024) if err != nil { t.Fatal(err) From b4cd10f61546e5b8316dd05b34af4dcd121f82d3 Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Wed, 18 Sep 2024 17:29:40 -0700 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7081f01cd..3bea6c343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The following emojis are used to highlight certain changes: ### Removed ### Fixed += `unixfs/hamt` Log error instead of panic if both link and shard are nil [#393](https://github.com/ipfs/boxo/pull/393) ### Security