panic in exec

[BenTheElder]

kubernetes/kubernetes#84122 (comment)

/priority critical-urgent
/assign

[BenTheElder]

/lifecycle active

[BenTheElder]

kind/pkg/exec/local.go

Lines 75 to 76 in 7dd62cd

	// TODO(BenTheElder): adding bytes.Buffer to both multiwriters might need
	// to be wrapped with a mutex

🤦‍♂ 🤦‍♂

[BenTheElder]

Go's standard library has:

// CombinedOutput runs the command and returns its combined standard
// output and standard error.
func (c *Cmd) CombinedOutput() ([]byte, error) {
	if c.Stdout != nil {
		return nil, errors.New("exec: Stdout already set")
	}
	if c.Stderr != nil {
		return nil, errors.New("exec: Stderr already set")
	}
	var b bytes.Buffer
	c.Stdout = &b
	c.Stderr = &b
	err := c.Run()
	return b.Bytes(), err
}

however, presumably this is a bit safer than a pair of MultiWriters.

[BenTheElder]

on the plus side, digging into what's going on here ... i think there's a good chance fixing this fixes #949

[BenTheElder]

So if cmd.Stdout and cmd.Stderr are identical, then Go will set up the actual fd for stdout and then set stderr to the same fd.

https://golang.org/src/os/exec/exec.go

func (c *Cmd) stdout() (f *os.File, err error) {
	return c.writerDescriptor(c.Stdout)
}

func (c *Cmd) stderr() (f *os.File, err error) {
	if c.Stderr != nil && interfaceEqual(c.Stderr, c.Stdout) {
		return c.childFiles[1], nil
	}
	return c.writerDescriptor(c.Stderr)
}

This is why CombinedOuput in the stdlib is safe.

When we set up an io.MultiWriter they are not the same, and get different fds.

In that case, we need to handle the writer receiving concurrent writes.

[BenTheElder]

Filed #972 with more details and a fix.

[BenTheElder]

This is a little tricky to intentionally reproduce, I believe #972 should prevent it, but we should keep our eyes peeled.