os/exec: Output and CombinedOutput methods should initialize the standard input with os.Stdin

[Bo0km4n]

What version of Go are you using (go version)?

go version go1.11.1 darwin/amd64

Does this issue reproduce with the latest release?

no

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/bo0km4n/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/bo0km4n/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/t2/zbxm3s595p7dk9w66mxvnn7c0000gn/T/go-build582580173=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I attempted to execute a binary containing a write system call using the os / exec package.
The code of c and the code of go are shown below.

#include<stdio.h>
#include<stdlib.h>
#include<unistd.h>


int main(){
  int result = write(0, "hello",6);
  if(result == -1){
    perror("write");
    exit(1);
  }
  printf("\nresult = %d", result);
  return 0;
}

package main

import (
	"fmt"
	"log"
	"os/exec"
)

func main() {
	target := "./c/test.o"

	cmd := exec.Command(target)
	execWriteBinary(cmd)
}

func execWriteBinary(cmd *exec.Cmd) {
	out, err := cmd.CombinedOutput()
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(string(out))
}

What did you expect to see?

hello
result = 6

What did you see instead?

2019/01/03 12:19:50 exit status 1
exit status 1

Current status

Current os/exec.go Output and CombinedOutput is like this

func (c *Cmd) Output() ([]byte, error) {
	if c.Stdout != nil {
		return nil, errors.New("exec: Stdout already set")
	}
	var stdout bytes.Buffer
	c.Stdout = &stdout

	captureErr := c.Stderr == nil
	if captureErr {
		c.Stderr = &prefixSuffixSaver{N: 32 << 10}
	}

	err := c.Run()
	if err != nil && captureErr {
		if ee, ok := err.(*ExitError); ok {
			ee.Stderr = c.Stderr.(*prefixSuffixSaver).Bytes()
		}
	}
	return stdout.Bytes(), err
}

// 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
}

In default, c.Stdin is setting with /dev/null, cause error was occurred in call write system call .

In the above code, only c.Stdout or both c.Stdout and c.Stderr are initialized by bytes.Buffer.

Proposal

I want to add initialize code like this

// 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

       + `c.Stdin = os.Stdin`

	err := c.Run()
	return b.Bytes(), err
}

[ianlancetaylor]

Why not just do that yourself before calling the Output or CombinedOutput methods?

I don't think we can change the default at this point.

[Bo0km4n]

Yes, that's right. But I want to know a reason that "Why exec package initializes stdin with /dev/null in default". If initialize stdin with os.Stdin, is there any problem?

[Bo0km4n]

I want to append code as follows to Output methods, light?

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")
	}

       // Added code start
       if c.Stdin == nil {
                c.Stdin = os.Stdin  
       }

	var b bytes.Buffer
	c.Stdout = &b
	c.Stderr = &b
	err := c.Run()
	return b.Bytes(), err
}

[ianlancetaylor]

There has to be some default. Because Go has a general philosophy of making the zero value valid, the default for the Stdin field is going to be nil. It seems natural to make nil mean that the child process is started with no standard input. If we added code as you suggest, then it would be awkward to request a child process with no standard input. It's always easy to do what you want, by setting the Stdin field to os.Stdin. That should always work.

It's not the case that setting Stdin to os.Stdin is normally correct. Most uses of the os/exec package do not set the Stdin field, because they do not expect the child process to read from standard input.

In any case, as I said above, I don't think we can change the default at this point. It would have an unexpected effect on too many existing and working programs. I'm going to close this issue. Please feel free to discuss further in a forum; see https://golang.org/wiki/Questions.

[Bo0km4n]

@ianlancetaylor I see. thanks! 👍