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

strconv: ParseInt() does not return an error when bitSize is out of bounds #21275

Closed
tillulen opened this issue Aug 2, 2017 · 13 comments
Closed
Milestone

Comments

@tillulen
Copy link

tillulen commented Aug 2, 2017

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
  • Version 1.8 on the playground

What operating system and processor architecture are you using (go env)?

λ go env
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=D:\Cache\Go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

Pass a negative bitSize and a "minus zero" string to strconv.ParseInt():

i, err := strconv.ParseInt("-0", 10, -1)
fmt.Println(i, err)

This prints:

0 <nil>

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:

i, err := strconv.ParseInt("0", 10, -1) // -1, strconv.ParseInt: parsing "0": value out of range

What did you see instead?

No error, err was nil.

@tillulen tillulen changed the title strconv.ParseInt() does not return an error when given a negative bitSize strconv.ParseInt() does not return an error when bitSize is out of bounds Aug 2, 2017
@tillulen
Copy link
Author

tillulen commented Aug 2, 2017

Hey @ALTree, thanks for your response. Did you delete your comment? I can no longer see it.

@ALTree
Copy link
Member

ALTree commented Aug 2, 2017

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 bitSize value.

strconv.ParseInt("123", 10, 65)

fails with an "out of range" error even if "123" clearly fits in 65 bits. It could be nice if ParseInt validated the BitSize parameter. Then we can change the doc to also explain what values are accepted.

Or at least we need to clarify in the doc what values of bitSize will actually work, because the current situation is that we error with "out of range" on inputs like 65 bits, which is confusing.

Also I noticed that we use bignums in the 1<<uint(bitSize) - 1 line so maybe the issue is not there.

@ALTree ALTree added this to the Go1.10 milestone Aug 2, 2017
@ALTree ALTree changed the title strconv.ParseInt() does not return an error when bitSize is out of bounds strconv: ParseInt() does not return an error when bitSize is out of bounds Aug 2, 2017
@tillulen
Copy link
Author

tillulen commented Aug 2, 2017

@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 bitSize < 0 or bitSize > 64, cutoff ends up being 0, which causes the strange effects observed above.

I don’t see any use of math/big in the entire atoi.go file. There seem to be no constants involved. Is there anything implicit going on with bignums there? I’m fairly new to Go, so I might have missed something obvious.

@ALTree
Copy link
Member

ALTree commented Aug 2, 2017

I was looking at line 93 maxVal = 1<<uint(bitSize) - 1 which does a similar thing but it's in ParseUint to see if this was the culprit for the difference between 0 and -0 (ParseInt calls ParseUint later), sorry for the mistake and the confusion.

EDIT: and ignore the rest of the comment, it wasn't useful, I wasn't reading the correct codepath.

@bcmills
Copy link
Contributor

bcmills commented Aug 3, 2017

I just want to note that #19624 would have caught the maxVal overflow in ParseUint and changed it from a silent-corruption bug to a panic with a clear trace.

It would also have caught the bad int-> uint conversion ParseInt when bitSize < 0, but would not have caught the bitSize > 64 case there because the overflow occurs in a << operation.

@bcmills
Copy link
Contributor

bcmills commented Aug 3, 2017

Actually, it would have caught that one too:
int64(cutoff - 1) overflows in the cutoff - 1 subexpression when the cutoff variable (a uint64) is 0, as is the case when bitSize overflows the valid range in the positive direction.

So #19624 would have caught this in all cases, I think.

@griesemer
Copy link
Contributor

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:

  • we should try hard to not make the code slower in relevant ways with the extra checks
  • it's not clear whether an invalid size should be an error or a panic: typically the bit size would be programmer given and is not arbitrary input, so an incorrect bit size is arguably a programmer error

Any comments regarding the 2nd caveat?

@tillulen
Copy link
Author

tillulen commented Aug 7, 2017

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 base is wrong and panics when bitSize is wrong, it doesn’t seem terribly consistent.

Does the standard library normally panic on programmer errors?

@bcmills
Copy link
Contributor

bcmills commented Aug 7, 2017

Does the standard library normally panic on programmer errors?

Yes, unless those errors are hard to avoid in practice.

That said, given that ParseInt already needs to return an error anyway, adding a panic seems excessive.

@rsc
Copy link
Contributor

rsc commented Aug 7, 2017

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.)

@griesemer griesemer assigned griesemer and martisch and unassigned griesemer Aug 8, 2017
@griesemer
Copy link
Contributor

Assigned to @martisch per discussion with him.

@gopherbot
Copy link

Change https://golang.org/cl/55139 mentions this issue: strconv: add bitsize range check to ParseUint

gopherbot pushed a commit that referenced this issue Aug 22, 2017
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>
@bradfitz
Copy link
Contributor

Looks like this was done? Closing. Let me know if I'm missing something.

@golang golang locked and limited conversation to collaborators Nov 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants