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

mark-rushakoff · 2020-10-30T15:50:45Z

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 60f42ea

The text was updated successfully, but these errors were encountered:

mark-rushakoff · 2020-10-30T16:38:01Z

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 · 2020-10-30T17:25:18Z

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 · 2020-10-30T17:43:15Z

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 · 2020-10-30T20:40:31Z

@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 · 2020-10-31T02:50:39Z

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:

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.
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 · 2020-10-31T04:06:06Z

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

ianlancetaylor · 2020-10-31T04:07:05Z

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 · 2020-11-01T19:50:25Z

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 · 2020-11-03T21:15:21Z

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

benhoyt · 2020-11-03T21:16:16Z

@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 · 2020-11-03T21:19:44Z

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

This is a partial revert of https://go-review.googlesource.com/c/go/+/248219 because we found that a non-trivial amount of code erroneously calls ParseFloat(s, 10) or even ParseFloat(s, 0) and expects it to work -- before that change was merged, ParseFloat accepted a bitSize of anything other than 32 or 64 to mean 64 (and ParseComplex was similar). So revert that behavior to avoid breaking people's code, and add tests for this. I may add a vet check to flag ParseFloat(s, not_32_or_64) in a later change. See #42297 for more details. Change-Id: I4bc0156bd74f67a39d5561b6e5fde3f2d20bd622 Reviewed-on: https://go-review.googlesource.com/c/go/+/267319 Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Go Bot <gobot@golang.org> Trust: Robert Griesemer <gri@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>

gopherbot · 2020-11-04T19:08:48Z

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

benhoyt · 2020-11-04T19:16:47Z

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

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).
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 · 2020-11-05T02:02:49Z

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

benhoyt · 2020-11-05T02:18:07Z

Sure. Done: #42389

benhoyt · 2020-11-07T03:59:39Z

Per discussion at #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 · 2020-11-07T04:00:31Z

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 · 2020-11-07T14:27:53Z

Thanks for your work on this.

ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Oct 30, 2020

ianlancetaylor added this to the Go1.16 milestone Oct 30, 2020

ianlancetaylor removed the release-blocker label Oct 30, 2020

ianlancetaylor added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Oct 30, 2020

gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 30, 2020

ianlancetaylor changed the title ~~strconv: new panic on tip, when parsing a float~~ strconv: add release note that ParseFloat now requires bit-size of either 32 or 64 Oct 30, 2020

ianlancetaylor added the help wanted label Oct 30, 2020

ianlancetaylor changed the title ~~strconv: add release note that ParseFloat now requires bit-size of either 32 or 64~~ strconv: permit values other than 32 and 64 in ParseFloat; add a vet check for those values Nov 3, 2020

ianlancetaylor removed the Documentation label Nov 3, 2020

benhoyt mentioned this issue Nov 5, 2020

proposal: cmd/vet: consider adding check for incorrect bitSize on strconv.ParseFloat (and similar functions) #42389

Closed

ianlancetaylor closed this as completed Nov 7, 2020

golang locked and limited conversation to collaborators Nov 7, 2021

gopherbot added the FrozenDueToAge label Nov 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mark-rushakoff commented Oct 30, 2020

mark-rushakoff commented Oct 30, 2020

ianlancetaylor commented Oct 30, 2020

mark-rushakoff commented Oct 30, 2020

benhoyt commented Oct 30, 2020

benhoyt commented Oct 31, 2020

ianlancetaylor commented Oct 31, 2020

ianlancetaylor commented Oct 31, 2020

benhoyt commented Nov 1, 2020

gopherbot commented Nov 3, 2020

benhoyt commented Nov 3, 2020

benhoyt commented Nov 3, 2020

gopherbot commented Nov 4, 2020

benhoyt commented Nov 4, 2020

ianlancetaylor commented Nov 5, 2020

benhoyt commented Nov 5, 2020

benhoyt commented Nov 7, 2020

benhoyt commented Nov 7, 2020

ianlancetaylor commented Nov 7, 2020

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

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

Comments

mark-rushakoff commented Oct 30, 2020

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

mark-rushakoff commented Oct 30, 2020

ianlancetaylor commented Oct 30, 2020

mark-rushakoff commented Oct 30, 2020

benhoyt commented Oct 30, 2020

benhoyt commented Oct 31, 2020

ianlancetaylor commented Oct 31, 2020

ianlancetaylor commented Oct 31, 2020

benhoyt commented Nov 1, 2020

gopherbot commented Nov 3, 2020

benhoyt commented Nov 3, 2020

benhoyt commented Nov 3, 2020

gopherbot commented Nov 4, 2020

benhoyt commented Nov 4, 2020

ianlancetaylor commented Nov 5, 2020

benhoyt commented Nov 5, 2020

benhoyt commented Nov 7, 2020

benhoyt commented Nov 7, 2020

ianlancetaylor commented Nov 7, 2020

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