internal/fuzz: fix internal error handling

This doesn't handle every possible scenario,
but improves the one we can control. For example,
if the worker panics for some reason, we have no
way of knowing whether the panic occurred in an
expected way (while executing the fuzz target) or
due to an internal error in the worker. So any
panic will still be treated as a crash.

However, if it fails due to some internal bug that
we know how to catch, then the error should be
reported to the user without a new crasher being
written to testdata.

This is very difficult to test. The reasons an
internal error would occur is because something went
very wrong, and we have a bug in our code (which is
why they were previously panics). So simulating
a problem like this in a test is not really feasible.

Fixes #48804

Change-Id: I334618f84eb4a994a8d17419551a510b1fdef071
Reviewed-on: https://go-review.googlesource.com/c/go/+/361115
Trust: Katie Hockman <katie@golang.org>
Run-TryBot: Katie Hockman <katie@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>

diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go
index 388675f71318164ca8e6d21bba03d130bbcce432..02efa7f84a4213cc3b7f082c5ff88ac88d9c997c 100644 (file)
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -153,7 +153,7 @@ func (w *worker) coordinate(ctx context.Context) error {
                                Warmup:       input.warmup,
                                CoverageData: input.coverageData,
                        }
-                       entry, resp, err := w.client.fuzz(ctx, input.entry, args)
+                       entry, resp, isInternalError, err := w.client.fuzz(ctx, input.entry, args)
                        canMinimize := true
                        if err != nil {
                                // Error communicating with worker.
@@ -167,14 +167,6 @@ func (w *worker) coordinate(ctx context.Context) error {
                                        // Report an error, but don't record a crasher.
                                        return fmt.Errorf("communicating with fuzzing process: %v", err)
                                }
-                               if w.waitErr == nil || isInterruptError(w.waitErr) {
-                                       // Worker stopped, either by exiting with status 0 or after being
-                                       // interrupted with a signal (not sent by coordinator). See comment in
-                                       // termC case above.
-                                       //
-                                       // Since we expect I/O errors around interrupts, ignore this error.
-                                       return nil
-                               }
                                if sig, ok := terminationSignal(w.waitErr); ok && !isCrashSignal(sig) {
                                        // Worker terminated by a signal that probably wasn't caused by a
                                        // specific input to the fuzz function. For example, on Linux,
@@ -183,6 +175,11 @@ func (w *worker) coordinate(ctx context.Context) error {
                                        // is closed. Don't record a crasher.
                                        return fmt.Errorf("fuzzing process terminated by unexpected signal; no crash will be recorded: %v", w.waitErr)
                                }
+                               if isInternalError {
+                                       // An internal error occurred which shouldn't be considered
+                                       // a crash.
+                                       return err
+                               }
                                // Unexpected termination. Set error message and fall through.
                                // We'll restart the worker on the next iteration.
                                // Don't attempt to minimize this since it crashed the worker.
@@ -567,6 +564,10 @@ type fuzzResponse struct {
        // Err is the error string caused by the value in shared memory, which is
        // non-empty if the value in shared memory caused a crash.
        Err string
+
+       // InternalErr is the error string caused by an internal error in the
+       // worker. This shouldn't be considered a crasher.
+       InternalErr string
 }
 
 // pingArgs contains arguments to workerServer.ping.
@@ -663,7 +664,8 @@ func (ws *workerServer) serve(ctx context.Context) error {
 func (ws *workerServer) fuzz(ctx context.Context, args fuzzArgs) (resp fuzzResponse) {
        if args.CoverageData != nil {
                if ws.coverageMask != nil && len(args.CoverageData) != len(ws.coverageMask) {
-                       panic(fmt.Sprintf("unexpected size for CoverageData: got %d, expected %d", len(args.CoverageData), len(ws.coverageMask)))
+                       resp.InternalErr = fmt.Sprintf("unexpected size for CoverageData: got %d, expected %d", len(args.CoverageData), len(ws.coverageMask))
+                       return resp
                }
                ws.coverageMask = args.CoverageData
        }
@@ -682,12 +684,14 @@ func (ws *workerServer) fuzz(ctx context.Context, args fuzzArgs) (resp fuzzRespo
                ws.memMu <- mem
        }()
        if args.Limit > 0 && mem.header().count >= args.Limit {
-               panic(fmt.Sprintf("mem.header().count %d already exceeds args.Limit %d", mem.header().count, args.Limit))
+               resp.InternalErr = fmt.Sprintf("mem.header().count %d already exceeds args.Limit %d", mem.header().count, args.Limit)
+               return resp
        }
 
        vals, err := unmarshalCorpusFile(mem.valueCopy())
        if err != nil {
-               panic(err)
+               resp.InternalErr = err.Error()
+               return resp
        }
 
        shouldStop := func() bool {
@@ -1027,7 +1031,7 @@ func (wc *workerClient) minimize(ctx context.Context, entryIn CorpusEntry, args
                entryOut.Parent = entryIn.Parent
                entryOut.Generation = entryIn.Generation
                if err != nil {
-                       panic(fmt.Sprintf("workerClient.minimize unmarshaling minimized value: %v", err))
+                       return CorpusEntry{}, minimizeResponse{}, fmt.Errorf("workerClient.minimize unmarshaling minimized value: %v", err)
                }
        } else {
                // Did not minimize, but the original input may still be interesting,
@@ -1039,40 +1043,43 @@ func (wc *workerClient) minimize(ctx context.Context, entryIn CorpusEntry, args
 }
 
 // fuzz tells the worker to call the fuzz method. See workerServer.fuzz.
-func (wc *workerClient) fuzz(ctx context.Context, entryIn CorpusEntry, args fuzzArgs) (entryOut CorpusEntry, resp fuzzResponse, err error) {
+func (wc *workerClient) fuzz(ctx context.Context, entryIn CorpusEntry, args fuzzArgs) (entryOut CorpusEntry, resp fuzzResponse, isInternalError bool, err error) {
        wc.mu.Lock()
        defer wc.mu.Unlock()
 
        mem, ok := <-wc.memMu
        if !ok {
-               return CorpusEntry{}, fuzzResponse{}, errSharedMemClosed
+               return CorpusEntry{}, fuzzResponse{}, true, errSharedMemClosed
        }
        mem.header().count = 0
        inp, err := CorpusEntryData(entryIn)
        if err != nil {
-               return CorpusEntry{}, fuzzResponse{}, err
+               return CorpusEntry{}, fuzzResponse{}, true, err
        }
        mem.setValue(inp)
        wc.memMu <- mem
 
        c := call{Fuzz: &args}
        callErr := wc.callLocked(ctx, c, &resp)
+       if resp.InternalErr != "" {
+               return CorpusEntry{}, fuzzResponse{}, true, errors.New(resp.InternalErr)
+       }
        mem, ok = <-wc.memMu
        if !ok {
-               return CorpusEntry{}, fuzzResponse{}, errSharedMemClosed
+               return CorpusEntry{}, fuzzResponse{}, true, errSharedMemClosed
        }
        defer func() { wc.memMu <- mem }()
        resp.Count = mem.header().count
 
        if !bytes.Equal(inp, mem.valueRef()) {
-               panic("workerServer.fuzz modified input")
+               return CorpusEntry{}, fuzzResponse{}, true, errors.New("workerServer.fuzz modified input")
        }
        needEntryOut := callErr != nil || resp.Err != "" ||
                (!args.Warmup && resp.CoverageData != nil)
        if needEntryOut {
                valuesOut, err := unmarshalCorpusFile(inp)
                if err != nil {
-                       panic(fmt.Sprintf("unmarshaling fuzz input value after call: %v", err))
+                       return CorpusEntry{}, fuzzResponse{}, true, fmt.Errorf("unmarshaling fuzz input value after call: %v", err)
                }
                wc.m.r.restore(mem.header().randState, mem.header().randInc)
                if !args.Warmup {
@@ -1098,7 +1105,7 @@ func (wc *workerClient) fuzz(ctx context.Context, entryIn CorpusEntry, args fuzz
                }
        }
 
-       return entryOut, resp, callErr
+       return entryOut, resp, false, callErr
 }
 
 // ping tells the worker to call the ping method. See workerServer.ping.

diff --git a/src/internal/fuzz/worker_test.go b/src/internal/fuzz/worker_test.go
index e32770b02ba86e527793db688207f936c53ad95e..c6f83fd08dd96cd372d49d2d393c2bbbca5e4773 100644 (file)
--- a/src/internal/fuzz/worker_test.go
+++ b/src/internal/fuzz/worker_test.go
@@ -96,7 +96,7 @@ func BenchmarkWorkerFuzz(b *testing.B) {
                        Limit:   int64(b.N) - i,
                        Timeout: workerFuzzDuration,
                }
-               _, resp, err := w.client.fuzz(context.Background(), entry, args)
+               _, resp, _, err := w.client.fuzz(context.Background(), entry, args)
                if err != nil {
                        b.Fatal(err)
                }