From ff12bea91c22bba93d3ffc3034d813d686bc7eeb Mon Sep 17 00:00:00 2001
From: Dmitry Vyukov <dvyukov@google.com>
Date: Mon, 23 Apr 2018 13:19:45 +0200
Subject: [PATCH] pkg/ipc: fix data race on config.Timeout

---
 pkg/ipc/ipc.go      | 54 ++++++++++++++++++++++++--------------------
 pkg/ipc/ipc_test.go | 55 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 24 deletions(-)

diff --git a/pkg/ipc/ipc.go b/pkg/ipc/ipc.go
index 320352a8..5128c61e 100644
--- a/pkg/ipc/ipc.go
+++ b/pkg/ipc/ipc.go
@@ -189,26 +189,6 @@ const (
 )
 
 func MakeEnv(config *Config, pid int) (*Env, error) {
-	const (
-		executorTimeout = 5 * time.Second
-		minTimeout      = executorTimeout + 2*time.Second
-	)
-	if config.Timeout == 0 {
-		// Executor protects against most hangs, so we use quite large timeout here.
-		// Executor can be slow due to global locks in namespaces and other things,
-		// so let's better wait than report false misleading crashes.
-		config.Timeout = time.Minute
-		if config.Flags&FlagUseForkServer == 0 {
-			// If there is no fork server, executor does not have internal timeout.
-			config.Timeout = executorTimeout
-		}
-	}
-	// IPC timeout must be larger then executor timeout.
-	// Otherwise IPC will kill parent executor but leave child executor alive.
-	if config.Flags&FlagUseForkServer != 0 && config.Timeout < minTimeout {
-		config.Timeout = minTimeout
-	}
-
 	var inf, outf *os.File
 	var inmem, outmem []byte
 	if config.Flags&FlagUseShmem != 0 {
@@ -486,6 +466,7 @@ func (env *Env) readOutCoverage(p *prog.Prog) (info []CallInfo, err0 error) {
 type command struct {
 	pid      int
 	config   *Config
+	timeout  time.Duration
 	cmd      *exec.Cmd
 	dir      string
 	readDone chan []byte
@@ -550,9 +531,10 @@ func makeCommand(pid int, bin []string, config *Config, inFile *os.File, outFile
 	}
 
 	c := &command{
-		pid:    pid,
-		config: config,
-		dir:    dir,
+		pid:     pid,
+		config:  config,
+		timeout: sanitizeTimeout(config),
+		dir:     dir,
 	}
 	defer func() {
 		if c != nil {
@@ -775,7 +757,7 @@ func (c *command) exec(opts *ExecOpts, progData []byte) (output []byte, failed,
 	done := make(chan bool)
 	hang := make(chan bool)
 	go func() {
-		t := time.NewTimer(c.config.Timeout)
+		t := time.NewTimer(c.timeout)
 		select {
 		case <-t.C:
 			c.abort()
@@ -852,3 +834,27 @@ func (c *command) exec(opts *ExecOpts, progData []byte) (output []byte, failed,
 	}
 	return
 }
+
+func sanitizeTimeout(config *Config) time.Duration {
+	const (
+		executorTimeout = 5 * time.Second
+		minTimeout      = executorTimeout + 2*time.Second
+	)
+	timeout := config.Timeout
+	if timeout == 0 {
+		// Executor protects against most hangs, so we use quite large timeout here.
+		// Executor can be slow due to global locks in namespaces and other things,
+		// so let's better wait than report false misleading crashes.
+		timeout = time.Minute
+		if config.Flags&FlagUseForkServer == 0 {
+			// If there is no fork server, executor does not have internal timeout.
+			timeout = executorTimeout
+		}
+	}
+	// IPC timeout must be larger then executor timeout.
+	// Otherwise IPC will kill parent executor but leave child executor alive.
+	if config.Flags&FlagUseForkServer != 0 && timeout < minTimeout {
+		timeout = minTimeout
+	}
+	return timeout
+}
diff --git a/pkg/ipc/ipc_test.go b/pkg/ipc/ipc_test.go
index 0d90f236..23f51f64 100644
--- a/pkg/ipc/ipc_test.go
+++ b/pkg/ipc/ipc_test.go
@@ -100,3 +100,58 @@ func TestExecute(t *testing.T) {
 		}
 	}
 }
+
+func TestParallel(t *testing.T) {
+	target, _, _, configFlags := initTest(t)
+	bin := buildExecutor(t, target)
+	defer os.Remove(bin)
+	cfg := &Config{
+		Executor: bin,
+		Flags:    configFlags,
+	}
+	const P = 10
+	errs := make(chan error, P)
+	for p := 0; p < P; p++ {
+		go func() {
+			env, err := MakeEnv(cfg, 0)
+			if err != nil {
+				errs <- fmt.Errorf("failed to create env: %v", err)
+				return
+			}
+			defer env.Close()
+			p := target.GenerateSimpleProg()
+			opts := &ExecOpts{}
+			output, info, failed, hanged, err := env.Exec(opts, p)
+			if err != nil {
+				errs <- fmt.Errorf("failed to run executor: %v", err)
+				return
+			}
+			if hanged {
+				errs <- fmt.Errorf("program hanged:\n%s", output)
+				return
+			}
+			if failed {
+				errs <- fmt.Errorf("program failed:\n%s", output)
+				return
+			}
+			if len(info) == 0 {
+				errs <- fmt.Errorf("no calls executed:\n%s", output)
+				return
+			}
+			if info[0].Errno != 0 {
+				errs <- fmt.Errorf("simple call failed: %v\n%s", info[0].Errno, output)
+				return
+			}
+			if len(output) != 0 {
+				errs <- fmt.Errorf("output on empty program")
+				return
+			}
+			errs <- nil
+		}()
+	}
+	for p := 0; p < P; p++ {
+		if err := <-errs; err != nil {
+			t.Fatal(err)
+		}
+	}
+}