-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
Comments
Change https://go.dev/cl/388354 mentions this issue: |
Change https://go.dev/cl/390095 mentions this issue: |
FYI @rolandshoemaker @bcmills, this broke the MIPS builders. See https://build.golang.org/. |
I'm going to reopen this for visibility. We shouldn't release 1.18 with a broken test regression. |
Change https://go.dev/cl/390214 mentions this issue: |
Change https://go.dev/cl/390418 mentions this issue: |
Change https://go.dev/cl/390424 mentions this issue: |
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>
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>
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>
Change https://go.dev/cl/390816 mentions this issue: |
…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>
What version of Go are you using (
go version
)?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
OutputWhat did you do?
Note: I had this happen with an actual Fuzz function I was writing that took
(float64, int, int)
and usingf.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 filein
~/go/go-build-cache/fuzz/
…/FuzzFormat/1b137f1e2faea57dd1c14f1c8ffc4bcf7e67ec00693d5f5e7f57ef840a50174a
with:Which causes
go fuzz
to produce the error every time no matter whatf.Add
calls are then used. Deleting the "bad" corpus file and excluding anyf.Add
lines with large/infinite values allowsgo fuzz
to run okay until/unless it tries another +Inf.Edit: I've also seen:
The text was updated successfully, but these errors were encountered: