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

fix: avoid out of bounds error when rendering short hashes #8318

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

Stebalien
Copy link
Member

The panic is caught higher up the stack so it can't crash the node, but it's still "not good".

@aschmahmann
Copy link
Contributor

I think the problem is slightly different. It looks like hash="" https://github.com/ipfs/go-ipfs/blob/cab67f6b6635b53bbf32680ee0da982e32bdcfee/core/corehttp/gateway_handler.go#L394 and when there's an error we just continue. What's going on there?

@Stebalien
Copy link
Member Author

Ah, I see. Yeah, that makes no sense.

(caught higher up but should still be fixed)
I believe we figured that these were for "informational purposes", but
really, we _should_ always be able to resolve names to CIDs. If we
can't, there's probably something wrong with the directory.
@Stebalien
Copy link
Member Author

I fixed it both ways.

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, it seems correct to me. I'd like to understand if there was some logic for why we didn't do this check before to make sure we don't introduce a different error (left as a comment)

Comment on lines +394 to +399
resolved, err := i.api.ResolvePath(r.Context(), ipath.Join(resolvedPath, dirit.Name()))
if err != nil {
internalWebError(w, err)
return
}
hash := resolved.Cid().String()
Copy link
Contributor

Choose a reason for hiding this comment

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

@lidel what was the idea behind "Path may not be resolved. Continue anyways.", it seems like we should just fail here as this PR does?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, how do we even reach the point where we error here? It seems like the UnixFS directory is telling us "here are the objects I have" and then we look for them and it errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, how do we even reach the point where we error here? It seems like the UnixFS directory is telling us "here are the objects I have" and then we look for them and it errors.

Invalid sharded directory. Or a subshard of a sharded directory that we can list but can't traverse.

@aschmahmann aschmahmann requested a review from lidel August 6, 2021 15:27
Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Not knowing the codebase before, SGTM. The previous code was definitely open to panics.

I guess you could just do the shortHash change to avoid panics. I don't have enough context to tell if err != nil should continue or stop.

@Stebalien Stebalien merged commit 7c76118 into master Aug 13, 2021
@Stebalien Stebalien deleted the fix/path-panic branch August 13, 2021 21:54
@Stebalien
Copy link
Member Author

Ok, I'm going to merge this for now. We can change it later if there is a better fix.

@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
fix: avoid out of bounds error when rendering short hashes

This commit was moved from ipfs/kubo@7c76118
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants