From c64639c3942814fe4ae6a7be0ee65a2ba4612c0b Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Mon, 20 Jan 2025 10:01:21 -1000 Subject: [PATCH 1/3] fix potential crahs in unixfs directory Fix potential crash in sizeBelowThreshold when modifying children in a directory. Closes #675 --- ipld/unixfs/io/directory.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/ipld/unixfs/io/directory.go b/ipld/unixfs/io/directory.go index 68160f0a9..14167f20a 100644 --- a/ipld/unixfs/io/directory.go +++ b/ipld/unixfs/io/directory.go @@ -489,7 +489,7 @@ func (d *HAMTDirectory) needsToSwitchToBasicDir(ctx context.Context, name string // until we either reach a value above the threshold (in that case no need // to keep counting) or an error occurs (like the context being canceled // if we take too much time fetching the necessary shards). -func (d *HAMTDirectory) sizeBelowThreshold(ctx context.Context, sizeChange int) (below bool, err error) { +func (d *HAMTDirectory) sizeBelowThreshold(ctx context.Context, sizeChange int) (bool, error) { if HAMTShardingSize == 0 { panic("asked to compute HAMT size with HAMTShardingSize option off (0)") } @@ -497,24 +497,37 @@ func (d *HAMTDirectory) sizeBelowThreshold(ctx context.Context, sizeChange int) // We don't necessarily compute the full size of *all* shards as we might // end early if we already know we're above the threshold or run out of time. partialSize := 0 + var err error + below := true // We stop the enumeration once we have enough information and exit this function. ctx, cancel := context.WithCancel(ctx) defer cancel() - for linkResult := range d.EnumLinksAsync(ctx) { + linkResults := d.EnumLinksAsync(ctx) + for linkResult := range linkResults { if linkResult.Err != nil { - return false, linkResult.Err + below = false + err = linkResult.Err + break } partialSize += linksize.LinkSizeFunction(linkResult.Link.Name, linkResult.Link.Cid) if partialSize+sizeChange >= HAMTShardingSize { // We have already fetched enough shards to assert we are // above the threshold, so no need to keep fetching. - return false, nil + below = false + break } } + if !below { + cancel() + for range linkResults { + } + return false, err + } + // We enumerated *all* links in all shards and didn't reach the threshold. return true, nil } From 436966785e6e6ede18481e9171a8615aafad144b Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Mon, 20 Jan 2025 10:07:57 -1000 Subject: [PATCH 2/3] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36ec53d73..b79632f96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ The following emojis are used to highlight certain changes: - `gateway` Fix redirect URLs for subdirectories with characters that need escaping. [#779](https://github.com/ipfs/boxo/pull/779) - `ipns` Fix `ipns` protobuf namespace conflicts by using full package name `github.com/ipfs/boxo/ipns/pb/record.proto` instead of the generic `record.proto` [#794](https://github.com/ipfs/boxo/pull/794) +- `unixfs` Fix possible crash when modifying directory [#798](https://github.com/ipfs/boxo/pull/798) ### Security From 9256fbeea8e9d3a3e559724995628ab9691f5e3d Mon Sep 17 00:00:00 2001 From: gammazero <11790789+gammazero@users.noreply.github.com> Date: Mon, 20 Jan 2025 12:22:45 -1000 Subject: [PATCH 3/3] simplify --- ipld/unixfs/io/directory.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ipld/unixfs/io/directory.go b/ipld/unixfs/io/directory.go index 14167f20a..ce6a6af9f 100644 --- a/ipld/unixfs/io/directory.go +++ b/ipld/unixfs/io/directory.go @@ -502,9 +502,8 @@ func (d *HAMTDirectory) sizeBelowThreshold(ctx context.Context, sizeChange int) // We stop the enumeration once we have enough information and exit this function. ctx, cancel := context.WithCancel(ctx) - defer cancel() - linkResults := d.EnumLinksAsync(ctx) + for linkResult := range linkResults { if linkResult.Err != nil { below = false @@ -514,21 +513,22 @@ func (d *HAMTDirectory) sizeBelowThreshold(ctx context.Context, sizeChange int) partialSize += linksize.LinkSizeFunction(linkResult.Link.Name, linkResult.Link.Cid) if partialSize+sizeChange >= HAMTShardingSize { - // We have already fetched enough shards to assert we are - // above the threshold, so no need to keep fetching. + // We have already fetched enough shards to assert we are above the + // threshold, so no need to keep fetching. below = false break } } + cancel() if !below { - cancel() + // Wait for channel to close so links are not being read after return. for range linkResults { } return false, err } - // We enumerated *all* links in all shards and didn't reach the threshold. + // Enumerated all links in all shards before threshold reached. return true, nil }