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

Jump to bottom

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

Closed
aclindsa opened this issue Apr 1, 2019 · 7 comments
Closed

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

aclindsa opened this issue Apr 1, 2019 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.13

Comments

@aclindsa
Copy link

aclindsa commented Apr 1, 2019

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
Copy link
Author

aclindsa commented Apr 1, 2019

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

@josharian josharian added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Apr 1, 2019
@josharian josharian added this to the Go1.13 milestone Apr 1, 2019
@gopherbot
Copy link

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

@cuonglm
Copy link
Member

cuonglm commented Apr 1, 2019

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

@aclindsa
Copy link
Author

aclindsa commented Apr 1, 2019

@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
Copy link
Member

cuonglm commented Apr 1, 2019

@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
Copy link
Author

aclindsa commented Apr 1, 2019

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
Copy link

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

@golang golang locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
4 participants
@josharian @aclindsa @cuonglm @gopherbot