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

CidFromReader should not wrap valid EOF return. #151

Merged
merged 4 commits into from
Apr 4, 2023
Merged

Conversation

gammazero
Copy link
Contributor

@gammazero gammazero commented Apr 1, 2023

When reading from an io.Reader that has no data, the io.EOF error should not be wrapped in ErrInvalidCid. This is not an invalid CID, and is not the same as a partial read which is indicated by io.ErrUnexpectedEOF.

This fix is needed because existing code that uses CidFromReader may check for the end of an input stream by if err == io.EOF instead of the preferred if errors.Is(err, io.EOF), and that code will break at runtime after upgrading to go-cid v0.4.0.

Fixes #152

When reading from an io.Reader that has no data, the io.EOF error should not be wrapped in ErrInvalidCid. This is not an invalid CID, and is not the same as a partial read which is indicated by io.ErrUnexpectedEOF.

This fix is needed because existing code that uses CidFromReader may check for the end of an input stream by `if err == io.EOF` instead of the preferred `if errors.Is(err, io.EOF)`, and that code break at runtime after upgrading to go-cid v0.4.0.
@gammazero gammazero requested review from rvagg and hacdias April 1, 2023 06:15
@rvagg
Copy link
Member

rvagg commented Apr 3, 2023

This is a tough call, arguably errors.Is(err, io.EOF) is the right way to deal with this but your code really has limited it to the first byte, so specifically the case where you have a zero-byte reader. I think this deserves some comments which I'll suggest inline.
Would you also mind adding a case for >1 byte that fails on the varint call but doesn't return an error - []byte{0x80, 0x01} should be good enough and should return an ErrInvalidCId wrapping an ErrUnexpectedEOF.

cid.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Apr 3, 2023

Unfortunately I can't suggest inline for the godoc comment because it's too far out of the diff, but here's what I suggest for clearning up that:

// It's recommended to supply a reader that buffers and implements io.ByteReader,
// as CidFromReader has to do many single-byte reads to decode varints.
// If the argument only implements io.Reader, single-byte Read calls are used instead.
//
// If the Reader is found to yield zero bytes, an io.EOF error is returned directly, in all
// other error cases, an ErrInvalidCid, wrapping the original error, is returned.
func CidFromReader(r io.Reader) (int, Cid, error) {

@gammazero
Copy link
Contributor Author

@rvagg Added test for ErrUnexpectedEOF and suggested comments.

@gammazero gammazero requested a review from rvagg April 3, 2023 22:26
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

nice

@github-actions
Copy link

github-actions bot commented Apr 4, 2023

Suggested version: v0.4.1

Comparing to: v0.4.0 (diff)

Changes in go.mod file(s):

(empty)

gorelease says:

# summary
Suggested version: v0.4.1

gocompat says:

Your branch is up to date with 'origin/master'.

Cutting a Release (and modifying non-markdown files)

This PR is modifying both version.json and non-markdown files.
The Release Checker is not able to analyse files that are not checked in to master. This might cause the above analysis to be inaccurate.
Please consider performing all the code changes in a separate PR before cutting the release.

Automatically created GitHub Release

A draft GitHub Release has been created.
It is going to be published when this PR is merged.
You can modify its' body to include any release notes you wish to include with the release.

@rvagg rvagg merged commit d46e7f2 into master Apr 4, 2023
@rvagg rvagg deleted the fix/from-reader-eof branch April 4, 2023 04:08
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.

CidFromReader wraps valid io.EOF in ErrInvalidCid
3 participants