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

UnmarshalBinary() panics if slice is not expected length. #231

Closed
jmalloc opened this issue Jun 9, 2021 · 6 comments
Closed

UnmarshalBinary() panics if slice is not expected length. #231

jmalloc opened this issue Jun 9, 2021 · 6 comments

Comments

@jmalloc
Copy link
Contributor

jmalloc commented Jun 9, 2021

I've just encountered a panic when testing invalid input to UnmarshalBinary(), due to the fact it access a subslice of data without doing any length checks (see https://github.com/shopspring/decimal/blob/master/decimal.go#L1156).

I suspect it should return an error instead, otherwise the user needs to understand the internal details of the binary format to validate this input before calling UnmarshalBinary().

@mwoss
Copy link
Member

mwoss commented Jun 10, 2021

Hi @jmalloc. Thanks for spotting this bug, this for sure needs to be improved.
#197 should cover this case, unfortunately, the author of this PR did not respond to my review, so I will take over this and try to merge it on the weekend to the master branch.

@jmalloc
Copy link
Contributor Author

jmalloc commented Jun 10, 2021

Ahh, I didn't notice that other PR, sorry. I've submitted #232 which hopefully addresses your concerns with #197.

@mwoss
Copy link
Member

mwoss commented Jun 11, 2021

No problem. Thanks for submitting the fresh PR :D
Unfortunately, I probably won't review it till the next weekend as I have only 6 days to finish my master's thesis and I have a lot things to do after 8-5 work. Sorry for that :<

@mwoss
Copy link
Member

mwoss commented Jun 22, 2021

@jmalloc I've merged your PR a moment ago, it looks really great. Thanks a lot for your contribution! :3

@jmalloc
Copy link
Contributor Author

jmalloc commented Jun 22, 2021

Excellent, thanks! I look forward to a new patch release ;)

@jmalloc
Copy link
Contributor Author

jmalloc commented Oct 14, 2021

I noticed this is tagged in 1.3.0, thanks!

@jmalloc jmalloc closed this as completed Oct 14, 2021
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

No branches or pull requests

2 participants