From 5ef7357b50015e91b096a4f14f554db78ba18b5f Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Mon, 1 Feb 2021 18:00:37 -0500 Subject: [PATCH] [dev.fuzz] internal/fuzz: refactor CorpusEntry type CorpusEntry is now a struct type with Name and Data fields. In the future, it may have more fields describing multiple values with different types added with f.Add. CorpusEntry must be the same type in testing and internal/fuzz. However, we don't want to export it from testing, and testing can't import internal/fuzz. We define it to be a type alias of a struct type instead of a defined type. We need to define it to the same thing in both places. We'll get a type error when building cmd/go if there's a difference. Change-Id: I9df6cd7aed67a6aa48b77ffb3a84bd302d2e5d94 Reviewed-on: https://go-review.googlesource.com/c/go/+/288534 Trust: Jay Conrod Run-TryBot: Jay Conrod TryBot-Result: Go Bot Reviewed-by: Katie Hockman --- src/internal/fuzz/fuzz.go | 61 ++++++++++++++++----------- src/internal/fuzz/worker.go | 8 ++-- src/testing/fuzz.go | 33 ++++++--------- src/testing/internal/testdeps/deps.go | 4 +- src/testing/testing.go | 10 ++--- 5 files changed, 59 insertions(+), 57 deletions(-) diff --git a/src/internal/fuzz/fuzz.go b/src/internal/fuzz/fuzz.go index f17bce35a2..451731ba93 100644 --- a/src/internal/fuzz/fuzz.go +++ b/src/internal/fuzz/fuzz.go @@ -39,7 +39,7 @@ import ( // // If a crash occurs, the function will return an error containing information // about the crash, which can be reported to the user. -func CoordinateFuzzing(ctx context.Context, parallel int, seed [][]byte, corpusDir, cacheDir string) (err error) { +func CoordinateFuzzing(ctx context.Context, parallel int, seed []CorpusEntry, corpusDir, cacheDir string) (err error) { if err := ctx.Err(); err != nil { return err } @@ -53,7 +53,7 @@ func CoordinateFuzzing(ctx context.Context, parallel int, seed [][]byte, corpusD return err } if len(corpus.entries) == 0 { - corpus.entries = []corpusEntry{{b: []byte{}}} + corpus.entries = []CorpusEntry{{Data: []byte{}}} } // TODO(jayconrod): do we want to support fuzzing different binaries? @@ -64,8 +64,8 @@ func CoordinateFuzzing(ctx context.Context, parallel int, seed [][]byte, corpusD c := &coordinator{ doneC: make(chan struct{}), - inputC: make(chan corpusEntry), - interestingC: make(chan corpusEntry), + inputC: make(chan CorpusEntry), + interestingC: make(chan CorpusEntry), crasherC: make(chan crasherEntry), errC: make(chan error), } @@ -141,7 +141,7 @@ func CoordinateFuzzing(ctx context.Context, parallel int, seed [][]byte, corpusD case crasher := <-c.crasherC: // A worker found a crasher. Write it to testdata and return it. - fileName, err := writeToCorpus(crasher.b, corpusDir) + fileName, err := writeToCorpus(crasher.Data, corpusDir) if err == nil { err = fmt.Errorf(" Crash written to %s\n%s", fileName, crasher.errMsg) } @@ -160,7 +160,7 @@ func CoordinateFuzzing(ctx context.Context, parallel int, seed [][]byte, corpusD // in the corpus. corpus.entries = append(corpus.entries, entry) if cacheDir != "" { - if _, err := writeToCorpus(entry.b, cacheDir); err != nil { + if _, err := writeToCorpus(entry.Data, cacheDir); err != nil { return err } } @@ -182,17 +182,32 @@ func CoordinateFuzzing(ctx context.Context, parallel int, seed [][]byte, corpusD } type corpus struct { - entries []corpusEntry + entries []CorpusEntry } -// TODO(jayconrod,katiehockman): decide whether and how to unify this type -// with the equivalent in testing. -type corpusEntry struct { - b []byte +// CorpusEntry represents an individual input for fuzzing. +// +// We must use an equivalent type in the testing and testing/internal/testdeps +// packages, but testing can't import this package directly, and we don't want +// to export this type from testing. Instead, we use the same struct type and +// use a type alias (not a defined type) for convenience. +type CorpusEntry = struct { + // Name is the name of the corpus file, if the entry was loaded from the + // seed corpus. It can be used with -run. For entries added with f.Add and + // entries generated by the mutator, Name is empty. + Name string + + // Data is the raw data loaded from a corpus file. + Data []byte + + // TODO(jayconrod,katiehockman): support multiple values of different types + // added with f.Add with a Values []interface{} field. We'll need marhsalling + // and unmarshalling functions, and we'll need to figure out what to do + // in the mutator. } type crasherEntry struct { - corpusEntry + CorpusEntry errMsg string } @@ -206,12 +221,12 @@ type coordinator struct { // inputC is sent values to fuzz by the coordinator. Any worker may receive // values from this channel. - inputC chan corpusEntry + inputC chan CorpusEntry // interestingC is sent interesting values by the worker, which is received // by the coordinator. Values are usually interesting because they // increase coverage. - interestingC chan corpusEntry + interestingC chan CorpusEntry // crasherC is sent values that crashed the code being fuzzed. These values // should be saved in the corpus, and we may want to stop fuzzing after @@ -231,33 +246,29 @@ type coordinator struct { // the same package at a different version or in a different module. // TODO(jayconrod,katiehockman): need a mechanism that can remove values that // aren't useful anymore, for example, because they have the wrong type. -func readCorpusAndCache(seed [][]byte, corpusDir, cacheDir string) (corpus, error) { +func readCorpusAndCache(seed []CorpusEntry, corpusDir, cacheDir string) (corpus, error) { var c corpus - for _, b := range seed { - c.entries = append(c.entries, corpusEntry{b: b}) - } + c.entries = append(c.entries, seed...) for _, dir := range []string{corpusDir, cacheDir} { - bs, err := ReadCorpus(dir) + entries, err := ReadCorpus(dir) if err != nil { return corpus{}, err } - for _, b := range bs { - c.entries = append(c.entries, corpusEntry{b: b}) - } + c.entries = append(c.entries, entries...) } return c, nil } // ReadCorpus reads the corpus from the testdata directory in this target's // package. -func ReadCorpus(dir string) ([][]byte, error) { +func ReadCorpus(dir string) ([]CorpusEntry, error) { files, err := ioutil.ReadDir(dir) if os.IsNotExist(err) { return nil, nil // No corpus to read } else if err != nil { return nil, fmt.Errorf("testing: reading seed corpus from testdata: %v", err) } - var corpus [][]byte + var corpus []CorpusEntry for _, file := range files { // TODO(jayconrod,katiehockman): determine when a file is a fuzzing input // based on its name. We should only read files created by writeToCorpus. @@ -271,7 +282,7 @@ func ReadCorpus(dir string) ([][]byte, error) { if err != nil { return nil, fmt.Errorf("testing: failed to read corpus file: %v", err) } - corpus = append(corpus, bytes) + corpus = append(corpus, CorpusEntry{Name: file.Name(), Data: bytes}) } return corpus, nil } diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go index 6d8dd188e1..8ea95438ca 100644 --- a/src/internal/fuzz/worker.go +++ b/src/internal/fuzz/worker.go @@ -104,7 +104,7 @@ func (w *worker) runFuzzing() error { w.memMu <- mem message := fmt.Sprintf("fuzzing process terminated unexpectedly: %v", w.waitErr) crasher := crasherEntry{ - corpusEntry: corpusEntry{b: value}, + CorpusEntry: CorpusEntry{Data: value}, errMsg: message, } w.coordinator.crasherC <- crasher @@ -119,7 +119,7 @@ func (w *worker) runFuzzing() error { inputC = nil // block new inputs until we finish with this one. go func() { args := fuzzArgs{Duration: workerFuzzDuration} - value, resp, err := w.client.fuzz(input.b, args) + value, resp, err := w.client.fuzz(input.Data, args) if err != nil { // Error communicating with worker. select { @@ -144,7 +144,7 @@ func (w *worker) runFuzzing() error { } else if resp.Err != "" { // The worker found a crasher. Inform the coordinator. crasher := crasherEntry{ - corpusEntry: corpusEntry{b: value}, + CorpusEntry: CorpusEntry{Data: value}, errMsg: resp.Err, } w.coordinator.crasherC <- crasher @@ -152,7 +152,7 @@ func (w *worker) runFuzzing() error { // Inform the coordinator that fuzzing found something // interesting (i.e. new coverage). if resp.Interesting { - w.coordinator.interestingC <- corpusEntry{b: value} + w.coordinator.interestingC <- CorpusEntry{Data: value} } // Continue fuzzing. diff --git a/src/testing/fuzz.go b/src/testing/fuzz.go index 4351704b58..7ef47872d0 100644 --- a/src/testing/fuzz.go +++ b/src/testing/fuzz.go @@ -48,17 +48,12 @@ type F struct { fuzzFunc func(f *F) // fuzzFunc is the function which makes up the fuzz target } -// corpus corpusEntry -type corpusEntry struct { - b []byte -} - -func bytesToCorpus(bytes [][]byte) []corpusEntry { - c := make([]corpusEntry, len(bytes)) - for i, b := range bytes { - c[i].b = b - } - return c +// corpusEntry is an alias to the same type as internal/fuzz.CorpusEntry. +// We use a type alias because we don't want to export this type, and we can't +// importing internal/fuzz from testing. +type corpusEntry = struct { + Name string + Data []byte } // Add will add the arguments to the seed corpus for the fuzz target. This will @@ -74,7 +69,7 @@ func (f *F) Add(args ...interface{}) { } switch v := args[0].(type) { case []byte: - f.corpus = append(f.corpus, corpusEntry{v}) + f.corpus = append(f.corpus, corpusEntry{Data: v}) // TODO: support other types default: panic("testing: Add only supports []byte currently") @@ -100,7 +95,7 @@ func (f *F) Fuzz(ff interface{}) { if err != nil { f.Fatal(err) } - f.corpus = append(f.corpus, bytesToCorpus(c)...) + f.corpus = append(f.corpus, c...) // TODO(jayconrod,katiehockman): dedupe testdata corpus with entries from f.Add var errStr string @@ -132,13 +127,9 @@ func (f *F) Fuzz(ff interface{}) { // Fuzzing is enabled, and this is the test process started by 'go test'. // Act as the coordinator process, and coordinate workers to perform the // actual fuzzing. - seed := make([][]byte, len(f.corpus)) - for i, e := range f.corpus { - seed[i] = e.b - } corpusTargetDir := filepath.Join(corpusDir, f.name) cacheTargetDir := filepath.Join(*fuzzCacheDir, f.name) - err := f.context.coordinateFuzzing(*fuzzDuration, *parallel, seed, corpusTargetDir, cacheTargetDir) + err := f.context.coordinateFuzzing(*fuzzDuration, *parallel, f.corpus, corpusTargetDir, cacheTargetDir) if err != nil { f.Fail() f.result = FuzzResult{Error: err} @@ -188,7 +179,7 @@ func (f *F) Fuzz(ff interface{}) { }, context: newTestContext(1, nil), } - go run(t, c.b) + go run(t, c.Data) <-t.signal if t.Failed() { f.Fail() @@ -281,9 +272,9 @@ func (r FuzzResult) String() string { type fuzzContext struct { runMatch *matcher fuzzMatch *matcher - coordinateFuzzing func(time.Duration, int, [][]byte, string, string) error + coordinateFuzzing func(time.Duration, int, []corpusEntry, string, string) error runFuzzWorker func(func([]byte) error) error - readCorpus func(string) ([][]byte, error) + readCorpus func(string) ([]corpusEntry, error) } // runFuzzTargets runs the fuzz targets matching the pattern for -run. This will diff --git a/src/testing/internal/testdeps/deps.go b/src/testing/internal/testdeps/deps.go index 2d0d7bac38..3d43170721 100644 --- a/src/testing/internal/testdeps/deps.go +++ b/src/testing/internal/testdeps/deps.go @@ -132,7 +132,7 @@ func (TestDeps) SetPanicOnExit0(v bool) { testlog.SetPanicOnExit0(v) } -func (TestDeps) CoordinateFuzzing(timeout time.Duration, parallel int, seed [][]byte, corpusDir, cacheDir string) error { +func (TestDeps) CoordinateFuzzing(timeout time.Duration, parallel int, seed []fuzz.CorpusEntry, corpusDir, cacheDir string) error { // Fuzzing may be interrupted with a timeout or if the user presses ^C. // In either case, we'll stop worker processes gracefully and save // crashers and interesting values. @@ -178,6 +178,6 @@ func (TestDeps) RunFuzzWorker(fn func([]byte) error) error { return nil } -func (TestDeps) ReadCorpus(dir string) ([][]byte, error) { +func (TestDeps) ReadCorpus(dir string) ([]fuzz.CorpusEntry, error) { return fuzz.ReadCorpus(dir) } diff --git a/src/testing/testing.go b/src/testing/testing.go index 39316122a6..e2abec2224 100644 --- a/src/testing/testing.go +++ b/src/testing/testing.go @@ -1361,11 +1361,11 @@ func (f matchStringOnly) ImportPath() string { return " func (f matchStringOnly) StartTestLog(io.Writer) {} func (f matchStringOnly) StopTestLog() error { return errMain } func (f matchStringOnly) SetPanicOnExit0(bool) {} -func (f matchStringOnly) CoordinateFuzzing(time.Duration, int, [][]byte, string, string) error { +func (f matchStringOnly) CoordinateFuzzing(time.Duration, int, []corpusEntry, string, string) error { return errMain } -func (f matchStringOnly) RunFuzzWorker(func([]byte) error) error { return errMain } -func (f matchStringOnly) ReadCorpus(string) ([][]byte, error) { return nil, errMain } +func (f matchStringOnly) RunFuzzWorker(func([]byte) error) error { return errMain } +func (f matchStringOnly) ReadCorpus(string) ([]corpusEntry, error) { return nil, errMain } // Main is an internal function, part of the implementation of the "go test" command. // It was exported because it is cross-package and predates "internal" packages. @@ -1408,9 +1408,9 @@ type testDeps interface { StartTestLog(io.Writer) StopTestLog() error WriteProfileTo(string, io.Writer, int) error - CoordinateFuzzing(time.Duration, int, [][]byte, string, string) error + CoordinateFuzzing(time.Duration, int, []corpusEntry, string, string) error RunFuzzWorker(func([]byte) error) error - ReadCorpus(string) ([][]byte, error) + ReadCorpus(string) ([]corpusEntry, error) } // MainStart is meant for use by tests generated by 'go test'. -- 2.48.1