syscall: fix a bug in the shuffling of file descriptors in StartProcess on Linux.

R=iant, iant, r, bradfitz
CC=golang-dev
https://golang.org/cl/8334044

2 files changed, 135 insertions, 2 deletions

diff --git a/src/pkg/os/exec/exec_test.go b/src/pkg/os/exec/exec_test.go
index 2467d29a11..bdfe69a21b 100644
--- a/src/pkg/os/exec/exec_test.go
+++ b/src/pkg/os/exec/exec_test.go
@@ -19,6 +19,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 )
 
 func helperCommand(s ...string) *Cmd {
@@ -194,6 +195,96 @@ func basefds() uintptr {
 	return n
 }
 
+func TestExtraFilesFDShuffle(t *testing.T) {
+	// syscall.StartProcess maps all the FDs passed to it in
+	// ProcAttr.Files (the concatenation of stdin,stdout,stderr and
+	// ExtraFiles) into consecutive FDs in the child, that is:
+	// Files{11, 12, 6, 7, 9, 3} should result in the file
+	// represented by FD 11 in the parent being made available as 0
+	// in the child, 12 as 1, etc.
+	//
+	// We want to test that FDs in the child do not get overwritten
+	// by one another as this shuffle occurs. The original implementation
+	// was buggy in that in some data dependent cases it would ovewrite
+	// stderr in the child with one of the ExtraFile members.
+	// Testing for this case is difficult because it relies on using
+	// the same FD values as that case. In particular, an FD of 3
+	// must be at an index of 4 or higher in ProcAttr.Files and
+	// the FD of the write end of the Stderr pipe (as obtained by
+	// StderrPipe()) must be the same as the size of ProcAttr.Files;
+	// therefore we test that the read end of this pipe (which is what
+	// is returned to the parent by StderrPipe() being one less than
+	// the size of ProcAttr.Files, i.e. 3+len(cmd.ExtraFiles).
+	//
+	// Moving this test case around within the overall tests may
+	// affect the FDs obtained and hence the checks to catch these cases.
+	npipes := 2
+	c := helperCommand("extraFilesAndPipes", strconv.Itoa(npipes+1))
+	rd, wr, _ := os.Pipe()
+	defer rd.Close()
+	if rd.Fd() != 3 {
+		t.Errorf("bad test value for test pipe: fd %d", rd.Fd())
+	}
+	stderr, _ := c.StderrPipe()
+	wr.WriteString("_LAST")
+	wr.Close()
+
+	pipes := make([]struct {
+		r, w *os.File
+	}, npipes)
+	data := []string{"a", "b"}
+
+	for i := 0; i < npipes; i++ {
+		r, w, err := os.Pipe()
+		if err != nil {
+			t.Fatalf("unexpected error creating pipe: %s", err)
+		}
+		pipes[i].r = r
+		pipes[i].w = w
+		w.WriteString(data[i])
+		c.ExtraFiles = append(c.ExtraFiles, pipes[i].r)
+		defer func() {
+			r.Close()
+			w.Close()
+		}()
+	}
+	// Put fd 3 at the end.
+	c.ExtraFiles = append(c.ExtraFiles, rd)
+
+	stderrFd := int(stderr.(*os.File).Fd())
+	if stderrFd != ((len(c.ExtraFiles) + 3) - 1) {
+		t.Errorf("bad test value for stderr pipe")
+	}
+
+	expected := "child: " + strings.Join(data, "") + "_LAST"
+
+	err := c.Start()
+	if err != nil {
+		t.Fatalf("Run: %v", err)
+	}
+	ch := make(chan string, 1)
+	go func(ch chan string) {
+		buf := make([]byte, 512)
+		n, err := stderr.Read(buf)
+		if err != nil {
+			t.Fatalf("Read: %s", err)
+			ch <- err.Error()
+		} else {
+			ch <- string(buf[:n])
+		}
+		close(ch)
+	}(ch)
+	select {
+	case m := <-ch:
+		if m != expected {
+			t.Errorf("Read: '%s' not '%s'", m, expected)
+		}
+	case <-time.After(5 * time.Second):
+		t.Errorf("Read timedout")
+	}
+	c.Wait()
+}
+
 func TestExtraFiles(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("no operating system support; skipping")
@@ -316,6 +407,13 @@ func TestExtraFilesRace(t *testing.T) {
 		}
 		la.Close()
 		lb.Close()
+		for _, f := range ca.ExtraFiles {
+			f.Close()
+		}
+		for _, f := range cb.ExtraFiles {
+			f.Close()
+		}
+
 	}
 }
 
@@ -449,6 +547,35 @@ func TestHelperProcess(*testing.T) {
 			}
 		}
 		os.Exit(0)
+	case "extraFilesAndPipes":
+		n, _ := strconv.Atoi(args[0])
+		pipes := make([]*os.File, n)
+		for i := 0; i < n; i++ {
+			pipes[i] = os.NewFile(uintptr(3+i), strconv.Itoa(i))
+		}
+		response := ""
+		for i, r := range pipes {
+			ch := make(chan string, 1)
+			go func(c chan string) {
+				buf := make([]byte, 10)
+				n, err := r.Read(buf)
+				if err != nil {
+					fmt.Fprintf(os.Stderr, "Child: read error: %v on pipe %d\n", err, i)
+					os.Exit(1)
+				}
+				c <- string(buf[:n])
+				close(c)
+			}(ch)
+			select {
+			case m := <-ch:
+				response = response + m
+			case <-time.After(5 * time.Second):
+				fmt.Fprintf(os.Stderr, "Child: Timeout reading from pipe: %d\n", i)
+				os.Exit(1)
+			}
+		}
+		fmt.Fprintf(os.Stderr, "child: %s", response)
+		os.Exit(0)
 	default:
 		fmt.Fprintf(os.Stderr, "Unknown command %q\n", cmd)
 		os.Exit(2)
diff --git a/src/pkg/syscall/exec_linux.go b/src/pkg/syscall/exec_linux.go
index a8dc672b8c..ddd946ed20 100644
--- a/src/pkg/syscall/exec_linux.go
+++ b/src/pkg/syscall/exec_linux.go
@@ -40,11 +40,18 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr
 		i      int
 	)
 
-	// guard against side effects of shuffling fds below.
+	// Guard against side effects of shuffling fds below.
+	// Make sure that nextfd is beyond any currently open files so
+	// that we can't run the risk of overwriting any of them.
 	fd := make([]int, len(attr.Files))
+	nextfd = len(attr.Files)
 	for i, ufd := range attr.Files {
+		if nextfd < int(ufd) {
+			nextfd = int(ufd)
+		}
 		fd[i] = int(ufd)
 	}
+	nextfd++
 
 	// About to call fork.
 	// No more allocation or calls of non-assembly functions.
@@ -143,7 +150,6 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr
 
 	// Pass 1: look for fd[i] < i and move those up above len(fd)
 	// so that pass 2 won't stomp on an fd it needs later.
-	nextfd = int(len(fd))
 	if pipe < nextfd {
 		_, _, err1 = RawSyscall(SYS_DUP2, uintptr(pipe), uintptr(nextfd), 0)
 		if err1 != 0 {