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: handle short read errors on string #381

Closed
wants to merge 3 commits into from

Conversation

brianshih1
Copy link
Contributor

A string is encoded as a long followed by that many bytes of UTF-8 encoded character data. If there aren't that many bytes of encoded character data, then it is an error.

For example, 0x08 decodes to long(4). Decoding the bytes[]byte{0x08} to a string should result in an error.

This PR wraps the EOF error, similar to the PR to fix array/maps.

reader.go Outdated
@@ -247,6 +247,10 @@ func (r *Reader) ReadBytes() []byte {
// ReadString reads a String from the Reader.
func (r *Reader) ReadString() string {
b := r.readBytes("string")
if r.Error != nil {
Copy link
Member

Choose a reason for hiding this comment

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

IMO, the fix needs to be more fundamental, in r.Read. r.Read is only ever called when an expected number of bytes should be read. Probably something like:

if r.head == r.tail {
  if !r.loadMore() {
    r.Error = io.ErrUnexpectedEOF
    return
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, working on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm looks like the changes broke a bunch of ocf tests somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrwiersma how are the files such as testdata/full-snappy.avro generated? I wonder if they have some invalid data in there.

Copy link
Member

Choose a reason for hiding this comment

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

No, OCF seems to be taking advantage of the no error from before. I can take care of this in a PR if you like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Appreciate it! Yeah sorry was stuck on this for a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you so much!

@brianshih1 brianshih1 closed this Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants