math/big: Subsequent calls to Rat.SetString set incorrect value

[aclindsa]

What version of Go are you using (go version)?

$ go version
go version devel +e4ba40030f Mon Mar 25 22:29:26 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

No, as far as I know, the issue is limited to the development branch and has not been released. I've bisected the issue to commit e4ba400.

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/me/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/me/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/lib/go"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build774704940=/tmp/go-build -gno-record-gcc-switches"

What did you do?

See https://play.golang.org/p/o3Oz63_fwGq (doesn't reproduce, because is using the released version) or add the following to src/math/big/rat_test.go with a current build from master:

func TestSubsequentSetStrings(t *testing.T) {
       a := new(Rat)
       a.SetString("-213.09")
       a.SetString("8.192")
       if a.FloatString(3) != "8.192" {
               t.Errorf("0) got %s want 8.192", a.FloatString(3))
       }
}

What did you expect to see?

8.192

What did you see instead?

1024.000

[aclindsa]

@griesemer I haven't had time to dig into the code yet, but my guess is that e4ba400 is missing a (re)initialization in some case or another. Let me know if you have any trouble reproducing this.

[gopherbot]

Change https://golang.org/cl/170158 mentions this issue: math/big: fix wrong pow5 compute in Rat.SetString

[cuonglm]

@aclindsa You're right, pow5 is computed wrongly, it's use previous z.b.abs instead of nat(nil).

[aclindsa]

@Gnouc Thanks for the quick response and fix!

Would it be worth adding a variant of TestRatSetString (in ratconv_test.go) that uses the same big.Rat between all the tests instead of creating a new one for each SetString call - to be more sure it will catch other variants of this issue that may take different code paths?

[cuonglm]

@aclindsa IMHO, a separated test (I added in the CL) is enough, 2 or more are the same. Let wait @griesemer to take a look at it.

[aclindsa]

Yes, thanks for adding the test. It will prevent this exact bug from occurring again. My question is whether a single pair of SetString() calls will adequately test all the code flows through this function, and therefore be likely to prevent similar, but slightly different, bugs in the future.

[gopherbot]

Change https://golang.org/cl/170641 mentions this issue: math/big: don't clobber shared underlying array in po5 computation