From ac58614190c9dd1ca0a1a47bf7a29a4a7c501e72 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Fri, 26 Feb 2021 12:50:48 -0500 Subject: [PATCH] [dev.fuzz] testing: use exit status 70 for worker errors (not crashes) If a worker process encounters an error communicating with the coordinator, or if the setup code reports an error with F.Fail before calling F.Fuzz, exit with status 70. The coordinator will report these errors and 'go test' will exit non-zero, but the coordinator won't record a crasher since the problem is not in the code being fuzzed. The coordinator also detects unexpected terminations before the worker calls F.Fuzz by sending a ping RPC. If the worker terminates before responding to the ping RPC, the coordinator won't record a crasher. Exit codes are chosen somewhat arbitrary, but in the Advanced Bash Scripting Guide, 70 is "internal software error" which is applicable here. 70 is also ASCII 'F'. Change-Id: I1e676e39a7b07c5664efaaa3221d055f55240fff Reviewed-on: https://go-review.googlesource.com/c/go/+/297033 Trust: Jay Conrod Trust: Katie Hockman Run-TryBot: Jay Conrod TryBot-Result: Go Bot Reviewed-by: Katie Hockman --- .../testdata/script/test_fuzz_mutate_fail.txt | 103 ++++++++++++++++++ src/internal/fuzz/worker.go | 69 +++++++++++- src/testing/fuzz.go | 14 ++- src/testing/testing.go | 6 +- 4 files changed, 183 insertions(+), 9 deletions(-) create mode 100644 src/cmd/go/testdata/script/test_fuzz_mutate_fail.txt diff --git a/src/cmd/go/testdata/script/test_fuzz_mutate_fail.txt b/src/cmd/go/testdata/script/test_fuzz_mutate_fail.txt new file mode 100644 index 0000000000..935c22a05e --- /dev/null +++ b/src/cmd/go/testdata/script/test_fuzz_mutate_fail.txt @@ -0,0 +1,103 @@ +# TODO(jayconrod): support shared memory on more platforms. +[!darwin] [!linux] [!windows] skip + +# Check that if a worker does not call F.Fuzz or calls F.Fail first, +# 'go test' exits non-zero and no crasher is recorded. + +[short] skip + +! go test -fuzz=FuzzReturn +! exists testdata + +! go test -fuzz=FuzzSkip +! exists testdata + +! go test -fuzz=FuzzFail +! exists testdata + +! go test -fuzz=FuzzPanic +! exists testdata + +! go test -fuzz=FuzzNilPanic +! exists testdata + +! go test -fuzz=FuzzGoexit +! exists testdata + +! go test -fuzz=FuzzExit +! exists testdata + +-- go.mod -- +module m + +go 1.17 +-- fuzz_fail_test.go -- +package fuzz_fail + +import ( + "flag" + "os" + "runtime" + "testing" +) + +func isWorker() bool { + f := flag.Lookup("test.fuzzworker") + if f == nil { + return false + } + get, ok := f.Value.(flag.Getter) + if !ok { + return false + } + return get.Get() == interface{}(true) +} + +func FuzzReturn(f *testing.F) { + if isWorker() { + return + } + f.Fuzz(func(*testing.T, []byte) {}) +} + +func FuzzSkip(f *testing.F) { + if isWorker() { + f.Skip() + } + f.Fuzz(func(*testing.T, []byte) {}) +} + +func FuzzFail(f *testing.F) { + if isWorker() { + f.Fail() + } + f.Fuzz(func(*testing.T, []byte) {}) +} + +func FuzzPanic(f *testing.F) { + if isWorker() { + panic("nope") + } + f.Fuzz(func(*testing.T, []byte) {}) +} + +func FuzzNilPanic(f *testing.F) { + if isWorker() { + panic(nil) + } + f.Fuzz(func(*testing.T, []byte) {}) +} + +func FuzzGoexit(f *testing.F) { + if isWorker() { + runtime.Goexit() + } + f.Fuzz(func(*testing.T, []byte) {}) +} + +func FuzzExit(f *testing.F) { + if isWorker() { + os.Exit(99) + } + f.Fuzz(func(*testing.T, []byte) {}) +} diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go index b44c321aac..1a590fad42 100644 --- a/src/internal/fuzz/worker.go +++ b/src/internal/fuzz/worker.go @@ -26,6 +26,11 @@ const ( // workerTimeoutDuration is the amount of time a worker can go without // responding to the coordinator before being stopped. workerTimeoutDuration = 1 * time.Second + + // workerExitCode is used as an exit code by fuzz worker processes after an internal error. + // This distinguishes internal errors from uncontrolled panics and other crashes. + // Keep in sync with internal/fuzz.workerExitCode. + workerExitCode = 70 ) // worker manages a worker process running a test binary. The worker object @@ -72,8 +77,29 @@ func (w *worker) runFuzzing() error { return err } - inputC := w.coordinator.inputC // set to nil when processing input - fuzzC := make(chan struct{}) // sent when we finish processing an input. + // inputC is set to w.coordinator.inputC when the worker is able to process + // input. It's nil at other times, so its case won't be selected in the + // event loop below. + var inputC chan CorpusEntry + + // A value is sent to fuzzC to tell the worker to prepare to process an input + // by setting inputC. + fuzzC := make(chan struct{}, 1) + + // Send the worker a message to make sure it can respond. + // Errors that occur before we get a response likely indicate that + // the worker did not call F.Fuzz or called F.Fail first. + // We don't record crashers for these errors. + pinged := false + go func() { + err := w.client.ping() + if err != nil { + w.stop() // trigger termC case below + return + } + pinged = true + fuzzC <- struct{}{} + }() // Main event loop. for { @@ -91,15 +117,20 @@ func (w *worker) runFuzzing() error { case <-w.termC: // Worker process terminated unexpectedly. + if !pinged { + w.stop() + return fmt.Errorf("worker terminated without fuzzing") + // TODO(jayconrod,katiehockman): record and return stderr. + } if isInterruptError(w.waitErr) { // Worker interrupted by SIGINT. See comment in doneC case. w.stop() return nil } - if w.waitErr == nil { - // Worker exited 0. + if exitErr, ok := w.waitErr.(*exec.ExitError); ok && exitErr.ExitCode() == workerExitCode { w.stop() - return fmt.Errorf("worker exited unexpectedly with status 0") + return fmt.Errorf("worker exited unexpectedly due to an internal failure") + // TODO(jayconrod,katiehockman): record and return stderr. } // Unexpected termination. Inform the coordinator about the crash. @@ -342,6 +373,7 @@ func RunFuzzWorker(ctx context.Context, fn func(CorpusEntry) error) error { // a minimalist RPC mechanism. Exactly one of its fields must be set to indicate // which method to call. type call struct { + Ping *pingArgs Fuzz *fuzzArgs } @@ -366,6 +398,12 @@ type fuzzResponse struct { Err string } +// pingArgs contains arguments to workerServer.ping. +type pingArgs struct{} + +// pingResponse contains results from workerServer.ping. +type pingResponse struct{} + // workerComm holds pipes and shared memory used for communication // between the coordinator process (client) and a worker process (server). // These values are unique to each worker; they are shared only with the @@ -439,6 +477,8 @@ func (ws *workerServer) serve(ctx context.Context) error { switch { case c.Fuzz != nil: resp = ws.fuzz(ctx, *c.Fuzz) + case c.Ping != nil: + resp = ws.ping(ctx, *c.Ping) default: return errors.New("no arguments provided for any call") } @@ -482,6 +522,12 @@ func (ws *workerServer) fuzz(ctx context.Context, args fuzzArgs) fuzzResponse { } } +// ping does nothing. The coordinator calls this method to ensure the worker +// has called F.Fuzz and can communicate. +func (ws *workerServer) ping(ctx context.Context, args pingArgs) pingResponse { + return pingResponse{} +} + // workerClient is a minimalist RPC client. The coordinator process uses a // workerClient to call methods in each worker process (handled by // workerServer). @@ -560,3 +606,16 @@ func (wc *workerClient) fuzz(valueIn []byte, args fuzzArgs) (valueOut []byte, re return valueOut, resp, err } + +// ping tells the worker to call the ping method. See workerServer.ping. +func (wc *workerClient) ping() error { + c := call{Ping: &pingArgs{}} + if err := wc.enc.Encode(c); err != nil { + return err + } + var resp pingResponse + if err := wc.dec.Decode(&resp); err != nil { + return err + } + return nil +} diff --git a/src/testing/fuzz.go b/src/testing/fuzz.go index 1a634dbe8b..c855379566 100644 --- a/src/testing/fuzz.go +++ b/src/testing/fuzz.go @@ -34,6 +34,11 @@ var ( corpusDir = "testdata/corpus" ) +// fuzzWorkerExitCode is used as an exit code by fuzz worker processes after an internal error. +// This distinguishes internal errors from uncontrolled panics and other crashes. +// Keep in sync with internal/fuzz.workerExitCode. +const fuzzWorkerExitCode = 70 + // InternalFuzzTarget is an internal type but exported because it is cross-package; // it is part of the implementation of the "go test" command. type InternalFuzzTarget struct { @@ -256,6 +261,9 @@ func (f *F) Fuzz(ff interface{}) { panic("testing: F.Fuzz called more than once") } f.fuzzCalled = true + if f.failed { + return + } f.Helper() // ff should be in the form func(*testing.T, ...interface{}) @@ -362,9 +370,9 @@ func (f *F) Fuzz(ff interface{}) { // Fuzzing is enabled, and this is a worker process. Follow instructions // from the coordinator. if err := f.fuzzContext.runFuzzWorker(run); err != nil { - // TODO(jayconrod,katiehockman): how should we handle a failure to - // communicate with the coordinator? Might be caused by the coordinator - // terminating early. + // Internal errors are marked with f.Fail; user code may call this too, before F.Fuzz. + // The worker will exit with fuzzWorkerExitCode, indicating this is a failure + // (and 'go test' should exit non-zero) but a crasher should not be recorded. f.Errorf("communicating with fuzzing coordinator: %v", err) } diff --git a/src/testing/testing.go b/src/testing/testing.go index 7ce794c5a8..2ad39f7137 100644 --- a/src/testing/testing.go +++ b/src/testing/testing.go @@ -1464,7 +1464,11 @@ func (m *M) Run() (code int) { } if !*isFuzzWorker && !fuzzingOk { fmt.Println("FAIL") - m.exitCode = 1 + if *isFuzzWorker { + m.exitCode = fuzzWorkerExitCode + } else { + m.exitCode = 1 + } return } -- 2.48.1