qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-17 14:04:48 -07:00
Code Releases Activity

testing: address feedback for dev.fuzz merge

Browse Source 
Based on comments in CL 348469.

Note that with this change, F.Fuzz no longer calls
runtime.Goexit. This simplifies our logic and makes F.Fuzz more
predictable.

Change-Id: I6c3c65b0e8e8f261621cbe2f17375e8164ef60a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/351316
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
This commit is contained in:
Jay Conrod 2021-09-21 16:30:06 -07:00
parent 1ce6fd03b8
commit d60ad1e068
12 changed files with 142 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
src/cmd/go/alldocs.go
View File

@ -293,8 +293,11 @@
// dependencies.
//
// The -fuzzcache flag causes clean to remove files stored in the Go build
// cache for fuzz testing. Files stored in source testdata directories
// are left in place.
// cache for fuzz testing. The fuzzing engine caches files that expand
// code coverage, so removing them may make fuzzing less effective until
// new inputs are found that provide the same coverage. These files are
// distinct from those stored in testdata directory; clean does not remove
// those files.
//
// For more about build flags, see 'go help build'.
//
@ -1854,6 +1857,13 @@
// See 'go help test' for details. Running 'go clean -testcache' removes
// all cached test results (but not cached build results).
//
// The go command also caches values used in fuzzing with 'go test -fuzz',
// specifically, values that expanded code coverage when passed to a
// fuzz function. These values are not used for regular building and
// testing, but they're stored in a subdirectory of the build cache.
// Running 'go clean -fuzzcache' removes all cached fuzzing values.
// This may make fuzzing less effective, temporarily.
//
// The GODEBUG environment variable can enable printing of debugging
// information about the state of the cache:
//

2
src/cmd/go/internal/cache/cache.go vendored
View File

@ -540,6 +540,8 @@ func (c *Cache) copyFile(file io.ReadSeeker, out OutputID, size int64) error {
// This directory is managed by the internal/fuzz package. Files in this
// directory aren't removed by the 'go clean -cache' command or by Trim.
// They may be removed with 'go clean -fuzzcache'.
//
// TODO(#48526): make Trim remove unused files from this directory.
func (c *Cache) FuzzDir() string {
return filepath.Join(c.dir, "fuzz")
}

9
src/cmd/go/internal/clean/clean.go
View File

@ -76,8 +76,11 @@ download cache, including unpacked source code of versioned
dependencies.
The -fuzzcache flag causes clean to remove files stored in the Go build
cache for fuzz testing. Files stored in source testdata directories
are left in place.
cache for fuzz testing. The fuzzing engine caches files that expand
code coverage, so removing them may make fuzzing less effective until
new inputs are found that provide the same coverage. These files are
distinct from those stored in testdata directory; clean does not remove
those files.
For more about build flags, see 'go help build'.
@ -220,7 +223,7 @@ func runClean(ctx context.Context, cmd *base.Command, args []string) {
}
if !cfg.BuildN {
if err := os.RemoveAll(fuzzDir); err != nil {
base.Errorf("go clean -fuzzcache: %v", err)
base.Errorf("go: %v", err)
}
}
}

7
src/cmd/go/internal/help/helpdoc.go
View File

@ -775,6 +775,13 @@ The go command also caches successful package test results.
See 'go help test' for details. Running 'go clean -testcache' removes
all cached test results (but not cached build results).
The go command also caches values used in fuzzing with 'go test -fuzz',
specifically, values that expanded code coverage when passed to a
fuzz function. These values are not used for regular building and
testing, but they're stored in a subdirectory of the build cache.
Running 'go clean -fuzzcache' removes all cached fuzzing values.
This may make fuzzing less effective, temporarily.
The GODEBUG environment variable can enable printing of debugging
information about the state of the cache:

5
src/cmd/go/internal/load/flag.go
View File

@ -22,9 +22,8 @@ var (
// that allows specifying different effective flags for different packages.
// See 'go help build' for more details about per-package flags.
type PerPackageFlag struct {
present bool
values []ppfValue
seenPackages map[*Package]bool // the packages for which the flags have already been set
present bool
values []ppfValue
}
// A ppfValue is a single <pattern>=<flags> per-package flag value.

18
src/cmd/go/internal/load/pkg.go
View File

@ -2630,20 +2630,10 @@ func (e *mainPackageError) ImportPath() string {
func setToolFlags(pkgs ...*Package) {
for _, p := range PackageList(pkgs) {
appendFlags(p, &p.Internal.Asmflags, &BuildAsmflags)
appendFlags(p, &p.Internal.Gcflags, &BuildGcflags)
appendFlags(p, &p.Internal.Ldflags, &BuildLdflags)
appendFlags(p, &p.Internal.Gccgoflags, &BuildGccgoflags)
}
}
func appendFlags(p *Package, flags *[]string, packageFlag *PerPackageFlag) {
if !packageFlag.seenPackages[p] {
if packageFlag.seenPackages == nil {
packageFlag.seenPackages = make(map[*Package]bool)
}
packageFlag.seenPackages[p] = true
*flags = append(*flags, packageFlag.For(p)...)
p.Internal.Asmflags = BuildAsmflags.For(p)
p.Internal.Gcflags = BuildGcflags.For(p)
p.Internal.Ldflags = BuildLdflags.For(p)
p.Internal.Gccgoflags = BuildGccgoflags.For(p)
}
}

2
src/cmd/go/internal/test/test.go
View File

@ -824,7 +824,7 @@ func runTest(ctx context.Context, cmd *base.Command, args []string) {
if testFuzz != "" && fuzzFlags != nil {
// Don't instrument packages which may affect coverage guidance but are
// unlikely to be useful. Most of these are used by the testing or
// internal/fuzz concurrently with fuzzing.
// internal/fuzz packages concurrently with fuzzing.
var fuzzNoInstrument = map[string]bool{
"context": true,
"internal/fuzz": true,

7
src/cmd/go/internal/work/init.go
View File

@ -61,8 +61,11 @@ func BuildInit() {
}
// FuzzInstrumentFlags returns compiler flags that enable fuzzing instrumation
// on supported platforms. On unsupported platforms, FuzzInstrumentFlags returns
// nil.
// on supported platforms.
//
// On unsupported platforms, FuzzInstrumentFlags returns nil, meaning no
// instrumentation is added. 'go test -fuzz' still works without coverage,
// but it generates random inputs without guidance, so it's much less effective.
func FuzzInstrumentFlags() []string {
// TODO: expand the set of supported platforms, with testing.
// Nothing about the instrumentation is OS specific, but only amd64 and arm64

25
src/cmd/go/testdata/script/test_fuzz.txt vendored
View File

@ -77,10 +77,15 @@ stdout 'f.Fuzz function'
stdout FAIL
stdout 'f.Fuzz function'
# Test that a call to f.Fatal after the Fuzz func is never executed.
go test fatal_after_fuzz_func_fuzz_test.go
stdout ok
! stdout FAIL
# Test that runtime.Goexit within the fuzz function is an error.
! go test goexit_fuzz_fn_fuzz_test.go
! stdout ^ok
stdout FAIL
# Test that a call to f.Fatal after the Fuzz func is executed.
! go test fatal_after_fuzz_func_fuzz_test.go
! stdout ok
stdout FAIL
# Test that missing *T in f.Fuzz causes a non-zero exit status.
! go test incomplete_fuzz_call_fuzz_test.go
@ -267,6 +272,18 @@ func Fuzz(f *testing.F) {
})
}
-- goexit_fuzz_fn_fuzz_test.go --
package goexit_fuzz_fn_fuzz
import "testing"
func Fuzz(f *testing.F) {
f.Add([]byte("aa"))
f.Fuzz(func(t *testing.T, b []byte) {
runtime.Goexit()
})
}
-- fatal_after_fuzz_func_fuzz_test.go --
package fatal_after_fuzz_func_fuzz

17
src/cmd/go/testdata/script/test_fuzz_mutate_crash.txt vendored
View File

@ -29,6 +29,11 @@ stdout 'testdata[/\\]fuzz[/\\]FuzzWithNilPanic[/\\]'
stdout 'runtime.Goexit'
go run check_testdata.go FuzzWithNilPanic
! go test -run=FuzzWithGoexit -fuzz=FuzzWithGoexit -fuzztime=100x -fuzzminimizetime=1000x
stdout 'testdata[/\\]fuzz[/\\]FuzzWithGoexit[/\\]'
stdout 'runtime.Goexit'
go run check_testdata.go FuzzWithGoexit
! go test -run=FuzzWithFail -fuzz=FuzzWithFail -fuzztime=100x -fuzzminimizetime=1000x
stdout 'testdata[/\\]fuzz[/\\]FuzzWithFail[/\\]'
go run check_testdata.go FuzzWithFail
@ -108,7 +113,8 @@ go 1.16
package fuzz_crash
import (
"os"
"os"
"runtime"
"testing"
)
@ -130,6 +136,15 @@ func FuzzWithNilPanic(f *testing.F) {
})
}
func FuzzWithGoexit(f *testing.F) {
f.Add([]byte("aa"))
f.Fuzz(func(t *testing.T, b []byte) {
if string(b) != "aa" {
runtime.Goexit()
}
})
}
func FuzzWithFail(f *testing.F) {
f.Add([]byte("aa"))
f.Fuzz(func(t *testing.T, b []byte) {

85
src/testing/fuzz.go
View File

@ -69,7 +69,7 @@ type F struct {
// from testdata.
corpus []corpusEntry
result FuzzResult
result fuzzResult
fuzzCalled bool
}
@ -290,17 +290,20 @@ var supportedTypes = map[reflect.Type]bool{
// whose remaining arguments are the types to be fuzzed.
// For example:
//
// f.Fuzz(func(t *testing.T, b []byte, i int) { ... })
// f.Fuzz(func(t *testing.T, b []byte, i int) { ... })
//
// This function should be fast, deterministic, and stateless.
// The following types are allowed: []byte, string, bool, byte, rune, float32,
// float64, int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64.
// More types may be supported in the future.
//
// No mutatable input arguments, or pointers to them, should be retained between
// executions of the fuzz function, as the memory backing them may be mutated
// during a subsequent invocation.
// This function sould be fast and deterministic, and its behavior should not
// depend on shared state. No mutatable input arguments, or pointers to them,
// should be retained between executions of the fuzz function, as the memory
// backing them may be mutated during a subsequent invocation.
//
// This is a terminal function which will terminate the currently running fuzz
// target by calling runtime.Goexit.
// To run any code after fuzzing stops, use (*F).Cleanup.
// When fuzzing, F.Fuzz does not return until a problem is found, time runs
// out (set with -fuzztime), or the test process is interrupted by a signal.
// F.Fuzz should be called exactly once unless F.Skip or F.Fail is called.
func (f *F) Fuzz(ff interface{}) {
if f.fuzzCalled {
panic("testing: F.Fuzz called more than once")
@ -440,7 +443,7 @@ func (f *F) Fuzz(ff interface{}) {
corpusTargetDir,
cacheTargetDir)
if err != nil {
f.result = FuzzResult{Error: err}
f.result = fuzzResult{Error: err}
f.Fail()
fmt.Fprintf(f.w, "%v\n", err)
if crashErr, ok := err.(fuzzCrashError); ok {
@ -469,16 +472,6 @@ func (f *F) Fuzz(ff interface{}) {
run(e)
}
}
// Record that the fuzz function (or coordinateFuzzing or runFuzzWorker)
// returned normally. This is used to distinguish runtime.Goexit below
// from panic(nil).
f.finished = true
// Terminate the goroutine. F.Fuzz should not return.
// We cannot call runtime.Goexit from a deferred function: if there is a
// panic, that would replace the panic value with nil.
runtime.Goexit()
}
func (f *F) report() {
@ -498,14 +491,14 @@ func (f *F) report() {
}
}
// FuzzResult contains the results of a fuzz run.
type FuzzResult struct {
// fuzzResult contains the results of a fuzz run.
type fuzzResult struct {
N int // The number of iterations.
T time.Duration // The total time taken.
Error error // Error is the error from the crash
}
func (r FuzzResult) String() string {
func (r fuzzResult) String() string {
s := ""
if r.Error == nil {
return s
@ -698,27 +691,28 @@ func fRunner(f *F, fn func(*F)) {
atomic.AddUint32(&numFailed, 1)
}
err := recover()
f.mu.RLock()
ok := f.skipped || f.failed || (f.fuzzCalled && f.finished)
f.mu.RUnlock()
if err == nil && !ok {
err = errNilPanicOrGoexit
if err == nil {
f.mu.RLock()
fuzzNotCalled := !f.fuzzCalled && !f.skipped && !f.failed
if !f.finished && !f.skipped && !f.failed {
err = errNilPanicOrGoexit
}
f.mu.RUnlock()
if fuzzNotCalled && err == nil {
f.Error("returned without calling F.Fuzz, F.Fail, or F.Skip")
}
}
// Use a deferred call to ensure that we report that the test is
// complete even if a cleanup function calls t.FailNow. See issue 41355.
// complete even if a cleanup function calls F.FailNow. See issue 41355.
didPanic := false
defer func() {
if didPanic {
return
if !didPanic {
// Only report that the test is complete if it doesn't panic,
// as otherwise the test binary can exit before the panic is
// reported to the user. See issue 41479.
f.signal <- true
}
if err != nil {
panic(err)
}
// Only report that the test is complete if it doesn't panic,
// as otherwise the test binary can exit before the panic is
// reported to the user. See issue 41479.
f.signal <- true
}()
// If we recovered a panic or inappropriate runtime.Goexit, fail the test,
@ -747,8 +741,9 @@ func fRunner(f *F, fn func(*F)) {
if len(f.sub) > 0 {
// Unblock inputs that called T.Parallel while running the seed corpus.
// T.Parallel has no effect while fuzzing, so this only affects fuzz
// targets run as normal tests.
// This only affects fuzz targets run as normal tests.
// While fuzzing, T.Parallel has no effect, so f.sub is empty, and this
// branch is not taken. f.barrier is nil in that case.
close(f.barrier)
// Wait for the subtests to complete.
for _, sub := range f.sub {
@ -776,11 +771,9 @@ func fRunner(f *F, fn func(*F)) {
f.start = time.Now()
fn(f)
// Code beyond this point is only executed if fn returned normally.
// That means fn did not call F.Fuzz or F.Skip. It should have called F.Fail.
// Code beyond this point will not be executed when FailNow or SkipNow
// is invoked.
f.mu.Lock()
defer f.mu.Unlock()
if !f.failed {
panic(f.name + " returned without calling F.Fuzz, F.Fail, or F.Skip")
}
f.finished = true
f.mu.Unlock()
}

57
src/testing/testing.go
View File

@ -146,21 +146,21 @@
//
// For example:
//
// func FuzzHex(f *testing.F) {
// for _, seed := range [][]byte{{}, {0}, {9}, {0xa}, {0xf}, {1, 2, 3, 4}} {
// f.Add(seed)
// }
// f.Fuzz(func(t *testing.T, in []byte) {
// enc := hex.EncodeToString(in)
// out, err := hex.DecodeString(enc)
// if err != nil {
// t.Fatalf("%v: decode: %v", in, err)
// func FuzzHex(f *testing.F) {
// for _, seed := range [][]byte{{}, {0}, {9}, {0xa}, {0xf}, {1, 2, 3, 4}} {
// f.Add(seed)
// }
// f.Fuzz(func(t *testing.T, in []byte) {
// enc := hex.EncodeToString(in)
// out, err := hex.DecodeString(enc)
// if err != nil {
// t.Fatalf("%v: decode: %v", in, err)
// }
// if !bytes.Equal(in, out) {
// t.Fatalf("%v: not equal after round trip: %v", in, out)
// }
// })
// }
// if !bytes.Equal(in, out) {
// t.Fatalf("%v: not equal after round trip: %v", in, out)
// }
// })
// }
//
// Seed inputs may be registered by calling F.Add or by storing files in the
// directory testdata/fuzz/<Name> (where <Name> is the name of the fuzz target)
@ -506,7 +506,7 @@ type common struct {
name string // Name of test or benchmark.
start time.Time // Time test or benchmark started
duration time.Duration
barrier chan bool // To signal parallel subtests they may start.
barrier chan bool // To signal parallel subtests they may start. Nil when T.Parallel is not present (B) or not usable (when fuzzing).
signal chan bool // To signal a test is done.
sub []*T // Queue of subtests to be run in parallel.
@ -628,13 +628,6 @@ func (c *common) frameSkip(skip int) runtime.Frame {
// and inserts the final newline if needed and indentation spaces for formatting.
// This function must be called with c.mu held.
func (c *common) decorate(s string, skip int) string {
if c.helperNames == nil {
c.helperNames = make(map[string]struct{})
for pc := range c.helperPCs {
c.helperNames[pcToName(pc)] = struct{}{}
}
}
frame := c.frameSkip(skip)
file := frame.File
line := frame.Line
@ -1280,14 +1273,6 @@ func tRunner(t *T, fn func(t *T)) {
err := recover()
signal := true
if err != nil && t.isFuzzing() {
t.Errorf("panic: %s\n%s\n", err, string(debug.Stack()))
t.mu.Lock()
t.finished = true
t.mu.Unlock()
err = nil
}
t.mu.RLock()
finished := t.finished
t.mu.RUnlock()
@ -1306,6 +1291,18 @@ func tRunner(t *T, fn func(t *T)) {
}
}
if err != nil && t.isFuzzing() {
prefix := "panic: "
if err == errNilPanicOrGoexit {
prefix = ""
}
t.Errorf("%s%s\n%s\n", prefix, err, string(debug.Stack()))
t.mu.Lock()
t.finished = true
t.mu.Unlock()
err = nil
}
// Use a deferred call to ensure that we report that the test is
// complete even if a cleanup function calls t.FailNow. See issue 41355.
didPanic := false