From: Jay Conrod <jayconrod@google.com>
Date: Fri, 15 Jan 2021 19:43:25 +0000 (-0500)
Subject: [dev.fuzz] internal/fuzz: handle SIGINT races gracefully
X-Git-Tag: go1.18beta1~1282^2~104
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=8e0584c327e429bd010edb28fb9fea6f68a4cccc;p=gostls13.git

[dev.fuzz] internal/fuzz: handle SIGINT races gracefully

A worker process may be terminated by SIGINT if it doesn't install the
signal handler before SIGINT is delivered. That's likely when TestMain
or the fuzz target setup take a long time. The coordinator now ignores
these errors.

Also, when testdeps.TestDeps.CoordinateFuzzing and RunFuzzWorker
return, they will send a value on the chan passed to signal.Notify
instead of closing it. This should have been obvious in hindsight, but
the signal handler could still send a value on that channel after
those functions return but before the process exits.

Change-Id: Iea2589115f1f9bb7415bb5e7911defee423e642e
Reviewed-on: https://go-review.googlesource.com/c/go/+/284292
Trust: Jay Conrod <jayconrod@google.com>
Reviewed-by: Katie Hockman <katie@golang.org>
---

diff --git a/src/internal/fuzz/sys_posix.go b/src/internal/fuzz/sys_posix.go
index ec27b4bf00..ecffa72755 100644
--- a/src/internal/fuzz/sys_posix.go
+++ b/src/internal/fuzz/sys_posix.go
@@ -73,3 +73,14 @@ func getWorkerComm() (comm workerComm, err error) {
 	}
 	return workerComm{fuzzIn: fuzzIn, fuzzOut: fuzzOut, mem: mem}, nil
 }
+
+// isInterruptError returns whether an error was returned by a process that
+// was terminated by an interrupt signal (SIGINT).
+func isInterruptError(err error) bool {
+	exitErr, ok := err.(*exec.ExitError)
+	if !ok || exitErr.ExitCode() >= 0 {
+		return false
+	}
+	status := exitErr.Sys().(syscall.WaitStatus)
+	return status.Signal() == syscall.SIGINT
+}
diff --git a/src/internal/fuzz/sys_unimplemented.go b/src/internal/fuzz/sys_unimplemented.go
index dbb380ef67..331b8761d0 100644
--- a/src/internal/fuzz/sys_unimplemented.go
+++ b/src/internal/fuzz/sys_unimplemented.go
@@ -29,3 +29,7 @@ func setWorkerComm(cmd *exec.Cmd, comm workerComm) {
 func getWorkerComm() (comm workerComm, err error) {
 	panic("not implemented")
 }
+
+func isInterruptError(err error) bool {
+	panic("not implemented")
+}
diff --git a/src/internal/fuzz/sys_windows.go b/src/internal/fuzz/sys_windows.go
index 286634c692..678ab0f0a3 100644
--- a/src/internal/fuzz/sys_windows.go
+++ b/src/internal/fuzz/sys_windows.go
@@ -131,3 +131,8 @@ func getWorkerComm() (comm workerComm, err error) {
 
 	return workerComm{fuzzIn: fuzzIn, fuzzOut: fuzzOut, mem: mem}, nil
 }
+
+func isInterruptError(err error) bool {
+	// TODO(jayconrod): implement
+	return false
+}
diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go
index 583e8f25c1..a10561a244 100644
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -80,19 +80,32 @@ func (w *worker) runFuzzing() error {
 		select {
 		case <-w.coordinator.doneC:
 			// All workers were told to stop.
-			return w.stop()
+			err := w.stop()
+			if isInterruptError(err) {
+				// Worker interrupted by SIGINT. This can happen if the worker receives
+				// SIGINT before installing the signal handler. That's likely if
+				// TestMain or the fuzz target setup takes a long time.
+				return nil
+			}
+			return err
 
 		case <-w.termC:
-			// Worker process terminated unexpectedly, so inform the coordinator
-			// that a crash occurred.
+			// Worker process terminated unexpectedly.
+			if isInterruptError(w.waitErr) {
+				// Worker interrupted by SIGINT. See comment in doneC case.
+				w.stop()
+				return nil
+			}
+
+			// Unexpected termination. Inform the coordinator about the crash.
+			// TODO(jayconrod,katiehockman): if -keepfuzzing, restart worker.
 			value := w.mem.valueCopy()
+			message := fmt.Sprintf("fuzzing process terminated unexpectedly: %v", w.waitErr)
 			crasher := crasherEntry{
 				corpusEntry: corpusEntry{b: value},
-				errMsg:      "fuzzing process crashed unexpectedly",
+				errMsg:      message,
 			}
 			w.coordinator.crasherC <- crasher
-
-			// TODO(jayconrod,katiehockman): if -keepfuzzing, restart worker.
 			err := w.stop()
 			if err == nil {
 				err = fmt.Errorf("worker exited unexpectedly")
diff --git a/src/testing/internal/testdeps/deps.go b/src/testing/internal/testdeps/deps.go
index dbc30ddc0f..2d0d7bac38 100644
--- a/src/testing/internal/testdeps/deps.go
+++ b/src/testing/internal/testdeps/deps.go
@@ -146,7 +146,7 @@ func (TestDeps) CoordinateFuzzing(timeout time.Duration, parallel int, seed [][]
 		<-interruptC
 		cancel()
 	}()
-	defer close(interruptC)
+	defer func() { interruptC <- os.Interrupt }()
 
 	err := fuzz.CoordinateFuzzing(ctx, parallel, seed, corpusDir, cacheDir)
 	if err == ctx.Err() {
@@ -169,7 +169,7 @@ func (TestDeps) RunFuzzWorker(fn func([]byte) error) error {
 		<-interruptC
 		cancel()
 	}()
-	defer close(interruptC)
+	defer func() { interruptC <- os.Interrupt }()
 
 	err := fuzz.RunFuzzWorker(ctx, fn)
 	if err == ctx.Err() {