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
strconv: ParseInt() does not return an error when bitSize is out of bounds #21275
Comments
Hey @ALTree, thanks for your response. Did you delete your comment? I can no longer see it. |
Yeah I deleted a message instead of editing it.. I was pointing out that there seems to be issues in the validation logic of the
fails with an "out of range" error even if "123" clearly fits in 65 bits. It could be nice if Or at least we need to clarify in the doc what values of Also I noticed that we use bignums in the |
@ALTree, I’m not sure I understand the bignums part. Are you looking at the master branch? It looks like this line is the culprit: cutoff := uint64(1 << uint(bitSize-1)) When I don’t see any use of |
I was looking at line 93 EDIT: and ignore the rest of the comment, it wasn't useful, I wasn't reading the correct codepath. |
I just want to note that #19624 would have caught the It would also have caught the bad |
Actually, it would have caught that one too: So #19624 would have caught this in all cases, I think. |
We should at least check that 0 <= bitSize <= 64. I believe the code as is would be correct for sizes other then 8, 16, 32, 64 (say 27 if one were so inclined). Caveats:
Any comments regarding the 2nd caveat? |
Not panicking seems the safer choice here, because some users might be doing interesting things to compute their bitSize based on something else. Let’s say you need to parse some input according to some user-defined format. Both the input format and the input become known only at runtime. (You may need this in a compiler, a development environment, or a reverse engineering tool.) You construct your ParseInt arguments based on your user’s input and then depend in part on ParseInt errors to detect when something goes wrong. If ParseInt returns an error when Does the standard library normally panic on programmer errors? |
Yes, unless those errors are hard to avoid in practice. That said, given that |
It seems fine to introduce a bad bit size error (for bitSize < 0 or > 64). It shouldn't be a panic. (We don't panic unless it's unavoidable, and there's an error result.) |
Assigned to @martisch per discussion with him. |
Change https://golang.org/cl/55139 mentions this issue: |
Return an error when a bitSize below 0 or above 64 is specified. Move bitSize 0 handling in ParseInt after the call to ParseUint to avoid a spill. AMD64: name old time/op new time/op delta Atoi 28.9ns ± 6% 27.4ns ± 6% -5.21% (p=0.002 n=20+20) AtoiNeg 24.6ns ± 2% 23.1ns ± 1% -6.04% (p=0.000 n=19+18) Atoi64 38.8ns ± 1% 38.0ns ± 1% -2.03% (p=0.000 n=17+20) Atoi64Neg 35.5ns ± 1% 34.3ns ± 1% -3.42% (p=0.000 n=19+20) Updates #21275 Change-Id: I70f0e4a16fa003f7ea929ca4ef56bd1a4181660b Reviewed-on: https://go-review.googlesource.com/55139 Reviewed-by: Robert Griesemer <gri@golang.org> Run-TryBot: Robert Griesemer <gri@golang.org>
Looks like this was done? Closing. Let me know if I'm missing something. |
I’m going over some code in the standard library to see what it is doing in various edge cases. I’m going to report any findings where erroneous input does not cause an error or where the docs are subtly different from what the real thing does.
I believe it is best to fix those things, however minor they appear, for the sake of improving robustness and security. Please let me know if the standard library is not supposed to perform thorough input validation and expects the caller to do so.
What version of Go are you using (
go version
)?go version go1.8.3 windows/amd64
What operating system and processor architecture are you using (
go env
)?What did you do?
Pass a negative bitSize and a "minus zero" string to strconv.ParseInt():
This prints:
See the runnable code on the Go Play Space or Go Playground.
What did you expect to see?
Expected ParseInt() to return an error. It does return an error for other input strings:
What did you see instead?
No error, err was
nil
.The text was updated successfully, but these errors were encountered: