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

core: BlockFetcher.(GetBlock, GetBlockByHeight) are susceptible to a panic due to insufficient nil checks #3816

Open
odeke-em opened this issue Oct 5, 2024 · 1 comment
Labels
external Issues created by non node team members needs:triage

Comments

@odeke-em
Copy link
Contributor

odeke-em commented Oct 5, 2024

If we examine this code

if res != nil && res.Block == nil {
return nil, fmt.Errorf("core/fetcher: block not found, hash: %s", hash.String())
}
return res.Block, nil
}
we notice that the first condition checks if res != nil and then if res.Block == nil. However by the time that we are checking if res != nil it can really be nil, yet on line 81 we are nakedly dereferncing res.Block

Suggestion

That code could be made much more whole and include a check for if res == nil || res.Block == nil { and then also add a test.

/cc @Wondertan @liamsi @cristaloleg

@github-actions github-actions bot added needs:triage external Issues created by non node team members labels Oct 5, 2024
@odeke-em odeke-em changed the title core: BlockFetcher.GetBlockByHeight is susceptible to a panic due to insufficient nil checks core: BlockFetcher.(GetBlock, GetBlockByHeight) are susceptible to a panic due to insufficient nil checks Oct 5, 2024
@Wondertan
Copy link
Member

Wondertan commented Oct 5, 2024

That's true, although in practise this case is not happening. Generally we think this result based api is confusing and error prone.

Care for a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members needs:triage
Projects
None yet
Development

No branches or pull requests

2 participants