[dev.fuzz] internal/fuzz: fix two bugs affecting windows

* Appending to the worker environment slice should reallocate it. On
  Windows, we pass handles through the environment, and concurrent
  workers were writing to the same memory, resulting in
  "The handle is invalid" errors.
* Instead of passing a handle to the temporary file, we pass its path
  to each worker instead. The worker is responsible for opening and
  closing the handle. Previously, all inheritable handles were
  inherited by all workers, even though only one was used. This
  prevented temporary files from being deleted after a worker stopped,
  because other workers would still have open handles to it.

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

diff --git a/src/internal/fuzz/fuzz.go b/src/internal/fuzz/fuzz.go
index aa121bf2a08e21de273442a9c7358750fcc06e2a..b8405622df8697ce316205dbbd1453545d9e7968 100644 (file)
--- a/src/internal/fuzz/fuzz.go
+++ b/src/internal/fuzz/fuzz.go
@@ -96,7 +96,7 @@ func CoordinateFuzzing(ctx context.Context, parallel int, seed []CorpusEntry, ty
                        dir:         dir,
                        binPath:     binPath,
                        args:        args,
-                       env:         env,
+                       env:         env[:len(env):len(env)], // copy on append to ensure workers don't overwrite each other.
                        coordinator: c,
                        memMu:       memMu,
                }, nil

diff --git a/src/internal/fuzz/sys_windows.go b/src/internal/fuzz/sys_windows.go
index 6d015c01952536266dc3bbb0c1a6057e09c48308..e1734af53c628700fffcb0a916df5b399ea17c76 100644 (file)
--- a/src/internal/fuzz/sys_windows.go
+++ b/src/internal/fuzz/sys_windows.go
@@ -9,8 +9,6 @@ import (
        "os"
        "os/exec"
        "reflect"
-       "strconv"
-       "strings"
        "syscall"
        "unsafe"
 )
@@ -19,7 +17,13 @@ type sharedMemSys struct {
        mapObj syscall.Handle
 }
 
-func sharedMemMapFile(f *os.File, size int, removeOnClose bool) (*sharedMem, error) {
+func sharedMemMapFile(f *os.File, size int, removeOnClose bool) (mem *sharedMem, err error) {
+       defer func() {
+               if err != nil {
+                       err = fmt.Errorf("mapping temporary file %s: %w", f.Name(), err)
+               }
+       }()
+
        // Create a file mapping object. The object itself is not shared.
        mapObj, err := syscall.CreateFileMapping(
                syscall.Handle(f.Fd()), // fhandle
@@ -86,12 +90,11 @@ func (m *sharedMem) Close() error {
 // run a worker process.
 func setWorkerComm(cmd *exec.Cmd, comm workerComm) {
        mem := <-comm.memMu
-       memFD := mem.f.Fd()
+       memName := mem.f.Name()
        comm.memMu <- mem
        syscall.SetHandleInformation(syscall.Handle(comm.fuzzIn.Fd()), syscall.HANDLE_FLAG_INHERIT, 1)
        syscall.SetHandleInformation(syscall.Handle(comm.fuzzOut.Fd()), syscall.HANDLE_FLAG_INHERIT, 1)
-       syscall.SetHandleInformation(syscall.Handle(memFD), syscall.HANDLE_FLAG_INHERIT, 1)
-       cmd.Env = append(cmd.Env, fmt.Sprintf("GO_TEST_FUZZ_WORKER_HANDLES=%x,%x,%x", comm.fuzzIn.Fd(), comm.fuzzOut.Fd(), memFD))
+       cmd.Env = append(cmd.Env, fmt.Sprintf("GO_TEST_FUZZ_WORKER_HANDLES=%x,%x,%q", comm.fuzzIn.Fd(), comm.fuzzOut.Fd(), memName))
 }
 
 // getWorkerComm returns communication channels in the worker process.
@@ -100,27 +103,21 @@ func getWorkerComm() (comm workerComm, err error) {
        if v == "" {
                return workerComm{}, fmt.Errorf("GO_TEST_FUZZ_WORKER_HANDLES not set")
        }
-       parts := strings.Split(v, ",")
-       if len(parts) != 3 {
-               return workerComm{}, fmt.Errorf("GO_TEST_FUZZ_WORKER_HANDLES has invalid value")
-       }
-       base := 16
-       bitSize := 64
-       handles := make([]syscall.Handle, len(parts))
-       for i, s := range parts {
-               h, err := strconv.ParseInt(s, base, bitSize)
-               if err != nil {
-                       return workerComm{}, fmt.Errorf("GO_TEST_FUZZ_WORKER_HANDLES has invalid value: %v", err)
-               }
-               handles[i] = syscall.Handle(h)
+       var fuzzInFD, fuzzOutFD uintptr
+       var memName string
+       if _, err := fmt.Sscanf(v, "%x,%x,%q", &fuzzInFD, &fuzzOutFD, &memName); err != nil {
+               return workerComm{}, fmt.Errorf("parsing GO_TEST_FUZZ_WORKER_HANDLES=%s: %v", v, err)
        }
 
-       fuzzIn := os.NewFile(uintptr(handles[0]), "fuzz_in")
-       fuzzOut := os.NewFile(uintptr(handles[1]), "fuzz_out")
-       tmpFile := os.NewFile(uintptr(handles[2]), "fuzz_mem")
+       fuzzIn := os.NewFile(fuzzInFD, "fuzz_in")
+       fuzzOut := os.NewFile(fuzzOutFD, "fuzz_out")
+       tmpFile, err := os.OpenFile(memName, os.O_RDWR, 0)
+       if err != nil {
+               return workerComm{}, fmt.Errorf("worker opening temp file: %w", err)
+       }
        fi, err := tmpFile.Stat()
        if err != nil {
-               return workerComm{}, err
+               return workerComm{}, fmt.Errorf("worker checking temp file size: %w", err)
        }
        size := int(fi.Size())
        if int64(size) != fi.Size() {

diff --git a/src/internal/fuzz/worker.go b/src/internal/fuzz/worker.go
index 1a590fad42da9d5b5146cb9cf31cf94b537798b6..4ccf469d60d3e78d794771ecfa4cd96c896352a0 100644 (file)
--- a/src/internal/fuzz/worker.go
+++ b/src/internal/fuzz/worker.go
@@ -219,7 +219,7 @@ func (w *worker) start() (err error) {
 
        cmd := exec.Command(w.binPath, w.args...)
        cmd.Dir = w.dir
-       cmd.Env = w.env
+       cmd.Env = w.env[:len(w.env):len(w.env)] // copy on append to ensure workers don't overwrite each other.
        // TODO(jayconrod): set stdout and stderr to nil or buffer. A large number
        // of workers may be very noisy, but for now, this output is useful for
        // debugging.