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>

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.

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)
 		}