Skip to content

Fixes #46: Buffer.GetBytes could cause panics#47

Merged
gafferongames merged 1 commit intomas-bandwidth:masterfrom
toqueteos:master
Aug 14, 2017
Merged

Fixes #46: Buffer.GetBytes could cause panics#47
gafferongames merged 1 commit intomas-bandwidth:masterfrom
toqueteos:master

Conversation

@toqueteos
Copy link
Contributor

@toqueteos toqueteos commented Aug 14, 2017

Explanation: Buffer.GetBytes only checks that the input size is less than the total buffer size so if we are near the end of the buffer and we ask for more bytes than there are left but still less than the total buffer size it'll panic.

Example: suppose we have a buffer b of size 4. If we do two calls to b.GetBytes(3), the second one will panic because we are asking for the last byte and two more.

I've also removed some unnecessary endlines in buffer_test.go

EDIT: Added a longer explanation here.

Copy link
Contributor

@wirepair wirepair left a comment

Choose a reason for hiding this comment

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

looks good to me.

@toqueteos toqueteos changed the title Fixes #42: Buffer.GetBytes could cause panics Fixes #46: Buffer.GetBytes could cause panics Aug 14, 2017
@toqueteos
Copy link
Contributor Author

toqueteos commented Aug 14, 2017

Sorry I linked #42 instead of #46.

EDIT: I've fixed the commit message

@toqueteos
Copy link
Contributor Author

Maybe instead of erroring out we should return all the remaining bytes and an error ErrEOB (end of buffer).

Another option is following io.Read's approach of returning the number of bytes read. But that's a breaking change on the API.

@gafferongames gafferongames merged commit d5c8358 into mas-bandwidth:master Aug 14, 2017
@gafferongames
Copy link
Contributor

Thanks!

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