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

[tillulen]

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]

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

[ALTree]

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.

[tillulen]

@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]

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]

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]

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]

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]

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]

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]

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