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

testing: fuzzing with float64 can produce errors and unparsable corpus files #51258

Closed
dchapes opened this issue Feb 18, 2022 · 9 comments
Closed
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dchapes
Copy link

dchapes commented Feb 18, 2022

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

$ go version
go version go1.18rc1 freebsd/amd64

Does this issue reproduce with the latest release?

Only relevant for Go1.18 beta and rc. Both beta2 and rc1 reproduce this.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/dchapes/go/go-build-cache"
GOENV="/home/dchapes/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS="-trimpath"
GOHOSTARCH="amd64"
GOHOSTOS="freebsd"
GOINSECURE=""
GOMODCACHE="/home/dchapes/go/pkg/mod"
GONOPROXY=""
GONOSUMDB="github.com/dchapes,hg.sr.ht/~dchapes"
GOOS="freebsd"
GOPATH="/home/dchapes/go"
GOPRIVATE=""
GOPROXY=""
GOROOT="/home/dchapes/sdk/go1.18rc1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/dchapes/sdk/go1.18rc1/pkg/tool/freebsd_amd64"
GOVCS=""
GOVERSION="go1.18rc1"
GCCGO="gccgo"
GOAMD64="v3"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/home/dchapes/go/src/dave-test/fuzz_float/go.mod"
GOWORK=""
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=/tmp/go-build2272238524=/tmp/go-build -gno-record-gcc-switches"

What did you do?

package fuzz_float

import (
	"math"
	"testing"
)

func FuzzFloat(f *testing.F) {
	f.Add(math.Inf(1))
	f.Fuzz(func(t *testing.T, f float64) {
		f = f + f
	})
}
$ go test -fuzz=. -fuzztime=10s
fuzz: elapsed: 0s, gathering baseline coverage: 0/2 completed
fuzz: elapsed: 0s, gathering baseline coverage: 1/2 completed
--- FAIL: FuzzFloat (0.01s)
    malformed line "float64(+Inf)": expected operation on int or float type
FAIL
exit status 1
FAIL    dave-test/fuzz_float    0.010s
Exit 1

Note: I had this happen with an actual Fuzz function I was writing that took (float64, int, int) and using f.Add(math.MaxFloat64, 0, 0). In that case it took a few fuzzing runs for the error to appear. That code being fuzzed does handle ±Inf specially so fuzzing would find those inputs interesting.

What did you expect to see?

Fuzzing to work and not create corpus files it can't parse.

What did you see instead?

The above error. In the included code since f.Add is used to add +Inf no bad corpus file is written, but in my actual use case I eventually got a corpus file
in ~/go/go-build-cache/fuzz//FuzzFormat/1b137f1e2faea57dd1c14f1c8ffc4bcf7e67ec00693d5f5e7f57ef840a50174a with:

go test fuzz v1
float64(+Inf)
int(-33)
int(82)

Which causes go fuzz to produce the error every time no matter what f.Add calls are then used. Deleting the "bad" corpus file and excluding any f.Add lines with large/infinite values allows go fuzz to run okay until/unless it tries another +Inf.

Edit: I've also seen:

    malformed line "float64(NaN)": literal value required for primitive type
@dmitshur dmitshur added fuzz Issues related to native fuzzing support NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 18, 2022
@dmitshur dmitshur added this to the Go1.18 milestone Feb 18, 2022
@dmitshur
Copy link
Contributor

CC @rolandshoemaker, @katiehockman.

@gopherbot
Copy link

Change https://go.dev/cl/388354 mentions this issue: internal/fuzz: handle Inf/NaN float values

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 4, 2022
@gopherbot
Copy link

Change https://go.dev/cl/390095 mentions this issue: [release-branch.go1.18] internal/fuzz: handle Inf/NaN float values

@mvdan
Copy link
Member

mvdan commented Mar 5, 2022

FYI @rolandshoemaker @bcmills, this broke the MIPS builders. See https://build.golang.org/.

@mvdan
Copy link
Member

mvdan commented Mar 6, 2022

I'm going to reopen this for visibility. We shouldn't release 1.18 with a broken test regression.

@mvdan mvdan reopened this Mar 6, 2022
@gopherbot
Copy link

Change https://go.dev/cl/390214 mentions this issue: internal/fuzz: fix TestUnmarshalMarshal on MIPS

@gopherbot
Copy link

Change https://go.dev/cl/390418 mentions this issue: [release-branch.go1.18] internal/fuzz: fix TestUnmarshalMarshal on MIPS

@gopherbot
Copy link

Change https://go.dev/cl/390424 mentions this issue: internal/fuzz: fix encoding for out-of-range ints and runes

gopherbot pushed a commit that referenced this issue Mar 8, 2022
Fixes #51258

Change-Id: I3c8b785ac912d66e1a6e2179625e6903032b8330
Reviewed-on: https://go-review.googlesource.com/c/go/+/388354
Reviewed-by: Bryan Mills <bcmills@google.com>
Trust: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 2b8aa2b)
Reviewed-on: https://go-review.googlesource.com/c/go/+/390095
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Mar 8, 2022
Previous value used in the float32 roundtrip used float32(math.NaN())-1
which caused the quiet/signal bit to flip, which seemed to break the
test on MIPS platforms. Instead switch to using float32(math.NaN())+1,
which preserves the bit and makes the test happy.

Possibly related to #37455
Fixes #51258

Change-Id: Ia85c649e89a5d02027c0ec197f0ff318aa819c19
Reviewed-on: https://go-review.googlesource.com/c/go/+/390214
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Trust: Roland Shoemaker <roland@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 63bd6f6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/390418
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Mar 8, 2022
Also switch float64 NaN encoding to use hexadecimal, and accept
hexadecimal encoding for all other integer types too. (That gives us
the flexibility to change the encodings in either direction in the
future without breaking earlier Go versions.)

Out-of-range runes encoded using "%q" were previously replaced with
the Unicode replacement charecter, losing their values.

Out-of-range ints and uints on 32-bit platforms were previously
rejected. Now they are wrapped instead: an “interesting” case with a
large int or uint found on a 64-bit platform likely remains
interesting on a 32-bit platform, even if the specific values differ.

To verify the above changes, I have made TestMarshalUnmarshal accept
(and check for) arbitrary differences between input and output, and
added tests cases that include values in valid but non-canonical
encodings.

I have also added round-trip fuzz tests in the opposite direction for
most of the types affected by this change, verifying that a marshaled
value unmarshals to the same bitwise value.

Updates #51258
Updates #51526
Fixes #51528

Change-Id: I7727a9d0582d81be0d954529545678a4374e88ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/390424
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/390816 mentions this issue: [release-branch.go1.18] internal/fuzz: fix encoding for out-of-range ints and runes

gopherbot pushed a commit that referenced this issue Mar 9, 2022
…ints and runes

Also switch float64 NaN encoding to use hexadecimal, and accept
hexadecimal encoding for all other integer types too. (That gives us
the flexibility to change the encodings in either direction in the
future without breaking earlier Go versions.)

Out-of-range runes encoded using "%q" were previously replaced with
the Unicode replacement charecter, losing their values.

Out-of-range ints and uints on 32-bit platforms were previously
rejected. Now they are wrapped instead: an “interesting” case with a
large int or uint found on a 64-bit platform likely remains
interesting on a 32-bit platform, even if the specific values differ.

To verify the above changes, I have made TestMarshalUnmarshal accept
(and check for) arbitrary differences between input and output, and
added tests cases that include values in valid but non-canonical
encodings.

I have also added round-trip fuzz tests in the opposite direction for
most of the types affected by this change, verifying that a marshaled
value unmarshals to the same bitwise value.

Updates #51258
Updates #51526
Fixes #51528

Change-Id: I7727a9d0582d81be0d954529545678a4374e88ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/390424
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 7419bb3)
Reviewed-on: https://go-review.googlesource.com/c/go/+/390816
Trust: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge fuzz Issues related to native fuzzing support NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: No status
Development

No branches or pull requests

4 participants