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

encoding/binary: Read(U)varint should return ErrUnexpectedEOF #33575

Open
Stebalien opened this issue Aug 9, 2019 · 3 comments
Open

encoding/binary: Read(U)varint should return ErrUnexpectedEOF #33575

Stebalien opened this issue Aug 9, 2019 · 3 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@Stebalien
Copy link

Stebalien commented Aug 9, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.7 linux/amd64

Description

binary.ReadUvarint and binary.ReadVarint currently return io.EOF when they encounter an EOF halfway to reading a varint.

This:

  1. Makes it difficult to distinguish between a graceful end of the stream and the stream ending in the middle of a varint as both cases will return an EOF. Technically, one can check if the partially read varint is non-zero but that's not documented and feels like a hack.
  2. Is a footgun for anyone using the standard if err != nil { return err } pattern. In this case, they'll return an EOF which usually signals "we're done", not "we read some bad data".
  3. Is an incorrect usage of the EOF error.

EOF documentation:

EOF is the error returned by Read when no more input is available. Functions should return EOF only to signal a graceful end of input. If the EOF occurs unexpectedly in a structured data stream, the appropriate error is either ErrUnexpectedEOF or some other error giving more detail.

ErrUnexpectedEOF documentation:

ErrUnexpectedEOF means that EOF was encountered in the middle of reading a fixed-size block or data structure.


Unfortunately, as pointed out by @ianlancetaylor, fixing this would technically be an API-breaking change.

@Stebalien
Copy link
Author

Stebalien commented Aug 9, 2019

Buggy function that inspired this report: https://github.com/gogo/protobuf/blob/28a6bbf47e48e0b2220b2a244750b660c83d4942/io/varint.go#L109-L126

This function will return EOF instead of some error if the stream closes before the end of the varint.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 12, 2019
@andybons andybons added this to the Unplanned milestone Aug 12, 2019
@andybons
Copy link
Member

andybons commented Aug 12, 2019

@griesemer

@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 12, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 12, 2019
@mxk
Copy link

mxk commented Dec 10, 2019

Checking that a partially read varint is non-zero doesn't work if those bits are all-zero. I just ran into this problem myself and definitely think that EOF after the first byte should be converted to ErrUnexpectedEOF as done by io.ReadFull and binary.Read.

I think any existing code that relies on EOF being returned after the first byte is more likely to be broken with the author unaware of the partial-read behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants