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

fmt: Sscanf accepts invalid boolean values #43306

Open
andydotxyz opened this issue Dec 21, 2020 · 7 comments
Open

fmt: Sscanf accepts invalid boolean values #43306

andydotxyz opened this issue Dec 21, 2020 · 7 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@andydotxyz
Copy link
Contributor

andydotxyz commented Dec 21, 2020

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

$ go version
1.15.5

Does this issue reproduce with the latest release?

Not checked 1.15.6 yet, sorry

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/andy/Library/Caches/go-build"
GOENV="/Users/andy/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/andy/Code/Go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/andy/Code/Go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.15.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.15.5/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/andy/Code/Go/src/fyne.io/fyne/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/rk/wps4hdmx73ldvbq2dbprd_3c0000gn/T/go-build914664370=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Tried to parse certain invalid boolean values using fmt.Sscanf and a "%t" format.
Using the following code to test certain input strings

	err, n := fmt.Sscanf("true", "%t", &val)
	fmt.Println("true:", val, n, err)

What did you expect to see?

true: true 1
tBu: true syntax error scanning boolean 0
trB: true syntax error scanning boolean 0
false: false 1
fRl: false syntax error scanning boolean 0
faR: false syntax error scanning boolean 0
foo: false syntax error scanning boolean 0

What did you see instead?

true: true 1
tBu: true 1
trB: true syntax error scanning boolean 0
false: false 1
fRl: false 1
faR: false syntax error scanning boolean 0
foo: false 1

@andydotxyz
Copy link
Contributor Author

looking at scan.go I see the function func (s *ss) scanBool(verb rune) bool

ends without an error:

	return false
}

should be

	s.error(boolError)
	return false
}

Additionally, true and false parsers look wrong,

	case 't', 'T':
		if s.accept("rR") && (!s.accept("uU") || !s.accept("eE")) {
			s.error(boolError)
		}
		return true

should be

	case 't', 'T':
		if !s.accept("rR") || !s.accept("uU") || !s.accept("eE") {
			s.error(boolError)
		}
		return true

@seankhliao
Copy link
Member

Can you provide a complete, runnable example? your example code and the expected input/output doesn't match

@andydotxyz
Copy link
Contributor Author

andydotxyz commented Dec 21, 2020

package main

import "fmt"

func main() {
	var val bool
	err, n := fmt.Sscanf("true", "%t", &val)
	fmt.Println("true:", val, n, err)
	err, n = fmt.Sscanf("tBu", "%t", &val)
	fmt.Println("tBu:", val, n, err)
	err, n = fmt.Sscanf("trB", "%t", &val)
	fmt.Println("trB:", val, n, err)

	err, n = fmt.Sscanf("false", "%t", &val)
	fmt.Println("false:", val, n, err)
	err, n = fmt.Sscanf("fRl", "%t", &val)
	fmt.Println("fRl:", val, n, err)
	err, n = fmt.Sscanf("faR", "%t", &val)
	fmt.Println("faR:", val, n, err)

	err, n = fmt.Sscanf("foo", "%t", &val)
	fmt.Println("foo:", val, n, err)
}

@seankhliao
Copy link
Member

I think you mixed up expected vs actual in the report. Additionally the value of val is kept between cases where there's a syntax error, a correct test case (with fixed n/err) would be:

func main() {
	ss := []string{"true", "tBu", "trB", "false", "fRl", "faR", "foo"}
	for _, s := range ss {
		var val bool
		n, err := fmt.Sscanf(s, "%t", &val)
		fmt.Println(s, val, n, err)
	}
}

with output

true true 1 <nil>
tBu true 1 <nil>
trB false 0 syntax error scanning boolean
false false 1 <nil>
fRl false 1 <nil>
faR false 0 syntax error scanning boolean
foo false 1 <nil>

The current logic accepts single letter T / F for true/false and only expecting the long form if the second letter also matches, allowing for things such as the following, changing it would be a breaking change

func main() {
	var v bool
	var w int
	fmt.Println(fmt.Sscanf("t1", "%t%d", &v, &w)) // 2 <nil>
	fmt.Println(v, w)                             // true 1
}

@andydotxyz
Copy link
Contributor Author

(updated the summary of the ticket, the outputs were the wrong way round)

Fixing the single item logic may indeed be breaking, I had not realised.

I guess the bigger issue, however, is that "garbage" will parse as a valid false value.

@mdempsky mdempsky changed the title fmt.Sscanf accepts invalid boolean values fmt: Sscanf accepts invalid boolean values Dec 22, 2020
@robpike
Copy link
Contributor

robpike commented Dec 22, 2020

The Scanf code doesn't try to handle all possible errors, and there is a comment in there to that effect. Scanf isn't a strong input verifier, and isn't meant to be. You should only use it when you want to do something simple like read integers from the user's input command line.

If you're looking for "true", look for "true", don't ask Scanf to verify it. Scanf is slow and tricky to use well for anything much beyond reading an int or float, so if you know the format of your input, you should use and/or write a proper lexer and/or parser.

That said, a clean, simple fix to Scanf that can be more rigorous about parsing booleans would be welcome, but both those adjectives matter. (And you can't add any dependencies to the package.) Assuming of course that being rigorous won't break existing code.

@robpike
Copy link
Contributor

robpike commented Dec 22, 2020

(For those following along at home, as I've said before, Scanf shouldn't even be in the library, being so much below the quality bar we would expect today. But the compatibility guarantee keeps it there.)

@cagedmantis cagedmantis added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 22, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants