Go: strconv: permit values other than 32 and 64 in ParseFloat; add a vet check for those values

[mark-rushakoff]

Does this issue reproduce with the latest release?

I have a unit test that has been passing for quite some time. It fails with go version devel +60f42ea61c Fri Oct 30 00:13:25 2020 +0000 darwin/amd64 but passes with go version devel +01efc9a3c5 Fri Oct 30 00:03:40 2020 +0000 darwin/amd64

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

go env Output

$ gotip env

GO111MODULE=""

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/mr/Library/Caches/go-build"

GOENV="/Users/mr/Library/Application Support/go/env"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOINSECURE=""

GOMODCACHE="/Users/mr/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="/Users/mr/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/Users/mr/gotip/src/github.com/golang/go"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/Users/mr/gotip/src/github.com/golang/go/pkg/tool/darwin_amd64"

GCCGO="gccgo"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="/Users/mr/go/src/github.com/influxdata/influxdb/go.mod"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ct/bl4_z3g51ks8239_r2k07v_40000gn/T/go-build565054434=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Check out github.com/influxdata/influxdb at commit f6a26ee2b99c1225cf1ca09487ea8fb45e515860 (approximately master at the time of writing this issue). Run go test ./models.

It was passing with go tip yesterday, but it is failing with go tip at go version devel +256d729c0b Fri Oct 30 15:26:28 2020 +0000 darwin/amd64.

What did you expect to see?

go version devel +01efc9a3c5 Fri Oct 30 00:03:40 2020 +0000 darwin/amd64
ok      github.com/influxdata/influxdb/v2/models    0.341s

What did you see instead?

$ gotip version && gotip test ./models
go version devel +60f42ea61c Fri Oct 30 00:13:25 2020 +0000 darwin/amd64
--- FAIL: TestParsePointMaxFloat64 (0.00s)
    points_test.go:694: ParsePoints("cpu,host=serverA,region=us-west value=9223372036854775807") mismatch. got unable to parse 'cpu,host=serverA,region=us-west value=179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.0': invalid float, exp nil
panic: runtime error: index out of range [0] with length 0 [recovered]
    panic: runtime error: index out of range [0] with length 0

goroutine 86 [running]:
testing.tRunner.func1.2(0x11f5ba0, 0xc000383968)
    /Users/mr/gotip/src/github.com/golang/go/src/testing/testing.go:1123 +0x332
testing.tRunner.func1(0xc000501c80)
    /Users/mr/gotip/src/github.com/golang/go/src/testing/testing.go:1126 +0x4b6
panic(0x11f5ba0, 0xc000383968)
    /Users/mr/gotip/src/github.com/golang/go/src/runtime/panic.go:965 +0x1b9
github.com/influxdata/influxdb/v2/models_test.TestParsePointMaxFloat64(0xc000501c80)
    /Users/mr/go/src/github.com/influxdata/influxdb/models/points_test.go:696 +0x654
testing.tRunner(0xc000501c80, 0x1216608)
    /Users/mr/gotip/src/github.com/golang/go/src/testing/testing.go:1173 +0xef
created by testing.(*T).Run
    /Users/mr/gotip/src/github.com/golang/go/src/testing/testing.go:1218 +0x2b3
FAIL    github.com/influxdata/influxdb/v2/models    0.147s
FAIL

Here is my git bisect log from the go repo:

git bisect start
# bad: [733c4af41d2e89d18cfb039e0107336a4c1ec220] go/types: add internal error codes
git bisect bad 733c4af41d2e89d18cfb039e0107336a4c1ec220
# good: [d9725f549f48ae6e4eaf70311f6827173453935b] syscall: add support for openbsd/mips64
git bisect good d9725f549f48ae6e4eaf70311f6827173453935b
# good: [5cc43c51c9929ce089ce2fc17a0f5631d21cd27d] cmd/compile: early devirtualization of interface method calls
git bisect good 5cc43c51c9929ce089ce2fc17a0f5631d21cd27d
# good: [f43e012084c4edd381d21c9988638535696775ea] strconv: make Eisel-Lemire handle long mantissas
git bisect good f43e012084c4edd381d21c9988638535696775ea
# bad: [60f42ea61cb7e1de8d54432d8fb9ab028b8a575d] strconv: fix incorrect bit size in ParseComplex; add tests
git bisect bad 60f42ea61cb7e1de8d54432d8fb9ab028b8a575d
# good: [01efc9a3c54f1b8fc772084e3311b6e1ccdfabec] strings: complete documentation of strings.Reader
git bisect good 01efc9a3c54f1b8fc772084e3311b6e1ccdfabec
# first bad commit: [60f42ea61cb7e1de8d54432d8fb9ab028b8a575d] strconv: fix incorrect bit size in ParseComplex; add tests

I don't yet have a minimal test case, but I may find some time today to reduce it. I'm filing the issue now in case anything jumps out to anyone else.

/cc @griesemer @minux @benhoyt for commit 60f42ea6

[mark-rushakoff]

I at least have all the source code in a single file reproducer at https://github.com/mark-rushakoff/goissue42297, but it is a nearly 3000 line file. I should be able to get it trimmed down quite a bit.

[ianlancetaylor]

What is happening here is that you are passing 10 as the bitSize argument to strconv.ParseFloat. The only valid values are 32 and 64. The code previously checked bitSize == 32 and treated any other value as being 64. Now it actually checks that the bit size is valid.

Since it has always been documented as accepting only 32 and 64, this change doesn't seem entirely unreasonable. It does need to be in the release notes, though.

[mark-rushakoff]

I see it now -- our code was masking the error that reported invalid bit size 10. I assume that our use of 10 was a copy-paste error from a ParseInt call. (It's been in our code at least since 2018, possibly even earlier.)

Thanks for quickly identifying what was wrong there and for fixing the issue title.

[benhoyt]

@ianlancetaylor I agree, this just makes incorrect code panic (did you mean 32 or 64?) and is already documented, so just a release notes change seems fine. I can create a CL with a small addition to https://github.com/golang/go/blob/master/doc/go1.16.html in the next few days.

[benhoyt]

Hmm, actually ... I've done some searching to see how common this is, and it seems like a fairly common mistake to use 10 (presumably because the second argument to ParseInt is often 10 for decimal) and, less commonly, 0. This is still invalid according to the documentation, but maybe we don't want to break that many usages for the sake of purity. Here are several usages of it from just a quick scan of code on my computer and then using Sourcegraph:

• Juju (my current work project) passes 10 here: https://github.com/juju/juju/blob/210f66c6baecd5bc9876507d0eac313d0f59a0de/core/series/supported.go#L103
• The Go protobuf decoder passes 0 here: https://github.com/golang/protobuf/blob/00998c7dd9d789e353c9518d13864b75cd23c55d/jsonpb/decode.go#L267
• Lots more instances of 10 (and that's not exhaustive -- last I checked Sourcegraph public search only indexed a subset of GitHub repos): https://sourcegraph.com/search?q=ParseFloat%28:%5Bfirst%5D%2C+10%29+lang:go+count:1000&patternType=structural
• Lots more instances of 0 (though most of these are protobuf): https://sourcegraph.com/search?q=ParseFloat%28:%5Bfirst%5D%2C+0%29+lang:go+count:1000&patternType=structural

It would be a bit different if ParseFloat panicked in these cases, but they return an error, meaning some programs will silently work incorrectly. Some programs may choose to fail hard on error, but others will assume the error means a parse error in the user input and fall back to a default value in the error case (see the Juju example), meaning program behavior will silently change. I don't think it's worth the risk.

I think there are two possibilities here:

1) Just revert the behavior of ParseFloat/ParseComplex to what they were before, so for example ParseFloat bitSize becomes 64 if it's anything other than 32. This is safe and non-invasive.
2) Change them to panic in this case instead. I think this would be reasonable, as it's almost always a programmer error to pass in a bitSize other than 32 or 64 (it's only if the bitSize were to come from user input that it wouldn't be). The string you're parsing likely comes from user input, so obviously errors in parsing would still return error.

I guess I lean towards option 1, as it's "safest". The Go 1 Compatibility Promise is strong with that one. Thoughts?

P. S. Vaguely relevant: https://xkcd.com/1172/

[ianlancetaylor]

I'm OK with accepting values other than 64, in which case we should have a test or two for that.

[ianlancetaylor]

With that many occurrences, we could also add take the approach of adding a vet check now, and then consider changing the code in a future release, and then in a still later release removing the vet check.

[benhoyt]

Sounds good. I'll upload a change that reverts the "error on invalid bitSize" behavior, and then look into adding a simple vet check (that might be kind of fun -- do you have any pointers or recent commits on adding a new vet check?)

[gopherbot]

Change https://golang.org/cl/267319 mentions this issue: strconv: revert ParseFloat/ParseComplex error on incorrect bitSize

[benhoyt]

@ianlancetaylor Change submitted for reverting the bitSize invalid error (and added tests for this): https://go-review.googlesource.com/c/go/+/267319 ... I intend to submit a change for the vet check later.

[benhoyt]

Oh also: you may want to update this issue title to reflect what we're actually doing.

[gopherbot]

Change https://golang.org/cl/267600 mentions this issue: go/analysis: add pass to check bitSize arguments to strconv calls

[benhoyt]

@ianlancetaylor go/analysis check added (will need to be added to go vet in a separate golang/go commit): https://go-review.googlesource.com/c/tools/+/267600 Two things I'm wondering about:

1) This just checks ParseFloat and ParseComplex. Do we want to add checks for other functions that take a bitSize as well? It'll be easy to add: AppendFloat (32, 64), FormatComplex (64, 128), FormatFloat (32, 64), ParseInt, ParseUint (0, 8, 16, 32, 64).
2) I'm pretty sure this satisfies the correctness and precision criteria here ... but does it satisfy the frequency criteria? Is the cost of running this check (I don't have a good sense of that at all, but presumably it's small) worth the benefit? This is flagging incorrect code, but it's probably not code that will actually have bugs in it (they may have used 10 instead of 64, but it'll still parse the number correctly and return a float64).

[ianlancetaylor]

@benhoyt Those are good questions. Can you open a new issue just for the vet check? Thanks.

[benhoyt]

Sure. Done: https://github.com/golang/go/issues/42389

[benhoyt]

Per discussion at https://github.com/golang/go/issues/42389, we've decided not to add any vet check. While ParseFloat(s, 10) is incorrect and strictly speaking a mistake, it's probably not going to cause issues or be a bug.

[benhoyt]

Oh, I guess I can't close this issue. @mark-rushakoff or @ianlancetaylor can you please close this now, seeing we've addressed the "permit other values" part and decided against the vet check.

[ianlancetaylor]

Thanks for your work on this.