Commit b493f0a8 authored by Cosmos Nicolaou's avatar Cosmos Nicolaou Committed by Rob Pike

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
parent 479b1241
......@@ -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)
......
......@@ -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 {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment