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

math/big: SetString does not handle malformed input correctly #17001

Closed
TomCoates8161 opened this issue Sep 6, 2016 · 5 comments
Closed

math/big: SetString does not handle malformed input correctly #17001

TomCoates8161 opened this issue Sep 6, 2016 · 5 comments
Milestone

Comments

@TomCoates8161
Copy link

Please answer these questions before submitting your issue. Thanks!

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

go version go1.5 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/tomc/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.5/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.5/libexec/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT=""
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

Using SetString to convert "4/3/2" to a *big.Rat claimed to succeed (which I think is the wrong behaviour) and returned 4/3 as the result (which is definitely the wrong behaviour)

https://play.golang.org/p/6iUww8dSlJ

What did you expect to see?

I expected the boolean ok in

_, ok := x.SetString("4/3/2")

to be false.

What did you see instead?

ok was true, and x was set to the rational number 4/3.

@quentinmit
Copy link
Contributor

This seems to be a limitation of nat.scan, which is called by SetString to read the denominator. It stops scanning at the first non-digit character, without returning an error.

SetString does check for extra characters after a decimal number, so one could argue it should also check after parsing a rational number. That said, we are also in the unfortunate place where there might be someone relying on this behavior. I suspect the best we can do is to simply document that SetString ignores any characters after the fraction.

/cc @griesemer

@quentinmit quentinmit added this to the Go1.8 milestone Sep 6, 2016
@griesemer
Copy link
Contributor

I would consider this a bug. For instance, Int.SetString checks that we have consumed all string characters. It looks like this is missing here, and the intent is definitively the same. The behavior should be consistent across all SetString methods.

@TomCoates8161
Copy link
Author

Also the current behaviour is mathematically wrong. See

https://play.golang.org/p/kULH591JrR

The rational number determined by the string "4/3/2" should probably be 2/3
-- being (4/3)/2 -- or could conceivably be 8/3 -- being 4/(3/2) -- but
certainly shouldn't be 4/3. Similarly, the rational number determined by
the string "5/2.5" should probably be undefined -- because the floating
point value 2.5 probably shouldn't be considered to be exact -- or could
conceivably be 2, but certainly shouldn't be 5/2. And the rational number
determined by the string "6/5abc" should probably be undefined, but
currently is 6/5.

So I feel that this is a bug.

Tom Coates
Professor of Mathematics
Imperial College London

@griesemer
Copy link
Contributor

@TomCoates8161 Agreed. The issue is that the parsing stops once a character is encountered that cannot possibly be part of a (simple) denominator. SetString should really return an error.

@gopherbot
Copy link

CL https://golang.org/cl/30472 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants