]> Cypherpunks repositories - gostls13.git/commitdiff
syscall: fix a bug in the shuffling of file descriptors in StartProcess on Linux.
authorCosmos Nicolaou <cnicolaou@google.com>
Tue, 30 Apr 2013 18:52:23 +0000 (11:52 -0700)
committerRob Pike <r@golang.org>
Tue, 30 Apr 2013 18:52:23 +0000 (11:52 -0700)
R=iant, iant, r, bradfitz
CC=golang-dev
https://golang.org/cl/8334044

src/pkg/os/exec/exec_test.go
src/pkg/syscall/exec_linux.go

index 2467d29a11eac9437a3d528af24e259b988432f9..bdfe69a21bb991c7922a2709bd224e551bb381f1 100644 (file)
@@ -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)
index a8dc672b8c908dfbfa3df241dfe0e3659395892b..ddd946ed20e81ae993383e6f1aea0bb76b5e17b5 100644 (file)
@@ -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 {