v23tests, modules and exec: make stdin redirection work and start work towards fixing https://github.com/veyron/release-issues/issues/1157
- stdin redirection was implemented in v23tests by firing of a goroutine
to read from an io.Reader and copy it to the child processes' stdin.
This is both fundamentally racy and inefficient. It's racy because
there is no guarantee that the goroutine will start reading and
writing before the child process has already read from its original
stdin, before the goroutine has even started. This is how I
ran into the problem. The race is fixed by changing lower level
APIs so that stdin can be correctly configured before the child
process starts.
- as explained in 1157, we need to distinguish between vanadium
child processes and non-vanadium ones, and to only engage
with the vanadium exec protocol with child processes that
we trust and that implement the protocol. I plan to change the
v23test and lower level APIs to support this in a followup CL.
This cl just makes a first step towards this, but is far from
complete.
Change-Id: Id01ea237bc1670071bb1893d67e057eac092f553
diff --git a/lib/modules/core/core_test.go b/lib/modules/core/core_test.go
index b775c1c..90a8a4e 100644
--- a/lib/modules/core/core_test.go
+++ b/lib/modules/core/core_test.go
@@ -211,7 +211,7 @@
func TestExec(t *testing.T) {
sh, cleanup := newShell(t)
defer cleanup()
- h, err := sh.StartExternalCommand(nil, []string{"/bin/sh", "-c", "echo hello world"}...)
+ h, err := sh.StartExternalCommand(nil, nil, []string{"/bin/sh", "-c", "echo hello world"}...)
if err != nil {
t.Fatalf("unexpected error: %s", err)
}
@@ -227,7 +227,7 @@
func TestExecWithEnv(t *testing.T) {
sh, cleanup := newShell(t)
defer cleanup()
- h, err := sh.StartExternalCommand([]string{"BLAH=hello world"}, "/bin/sh", "-c", "printenv BLAH")
+ h, err := sh.StartExternalCommand(nil, []string{"BLAH=hello world"}, "/bin/sh", "-c", "printenv BLAH")
if err != nil {
t.Fatalf("unexpected error: %s", err)
}
diff --git a/lib/modules/exec.go b/lib/modules/exec.go
index 2c65c60..9775a71 100644
--- a/lib/modules/exec.go
+++ b/lib/modules/exec.go
@@ -23,10 +23,12 @@
entryPoint string
handle *vexec.ParentHandle
sh *Shell
+ childStdin io.Reader
stderr *os.File
stdout io.ReadCloser
stdin io.WriteCloser
procErrCh chan error
+ external bool
}
func testFlags() []string {
@@ -70,8 +72,8 @@
return &execHandle{name: name, entryPoint: shellEntryPoint + "=" + name, procErrCh: make(chan error, 1)}
}
-func newExecHandleForExternalCommand(name string) command {
- return &execHandle{name: name, procErrCh: make(chan error, 1)}
+func newExecHandleForExternalCommand(name string, stdin io.Reader) command {
+ return &execHandle{name: name, external: true, childStdin: stdin, procErrCh: make(chan error, 1)}
}
func (eh *execHandle) Stdout() io.Reader {
@@ -129,6 +131,7 @@
cmd := exec.Command(cmdPath, newargs[1:]...)
cmd.Env = newenv
+
stderr, err := newLogfile("stderr", eh.name)
if err != nil {
return nil, err
@@ -142,9 +145,18 @@
// while respecting the timeout.
stdout := newRW()
cmd.Stdout = stdout
- stdin, err := cmd.StdinPipe()
- if err != nil {
- return nil, err
+
+ // If we have an explicit stdin to pass to the child, use that,
+ // otherwise create a pipe and return the write side of that pipe
+ // in the handle.
+ if eh.childStdin != nil {
+ cmd.Stdin = eh.childStdin
+ } else {
+ stdin, err := cmd.StdinPipe()
+ if err != nil {
+ return nil, err
+ }
+ eh.stdin = stdin
}
config := vexec.NewConfig()
serialized, err := sh.config.Serialize()
@@ -159,10 +171,12 @@
defer agentfd.Close()
}
+ // TODO(cnicolaou): for external commands, vexec should either not be
+ // used or it should taken an option to not use its protocol, and in
+ // particular to share secrets with children.
handle := vexec.NewParentHandle(cmd, vexec.ConfigOpt{Config: config})
eh.stdout = stdout
eh.stderr = stderr
- eh.stdin = stdin
eh.handle = handle
eh.cmd = cmd
vlog.VI(1).Infof("Start: %q stderr: %s", eh.name, stderr.Name())
@@ -198,7 +212,9 @@
defer eh.mu.Unlock()
vlog.VI(1).Infof("Shutdown: %q", eh.name)
defer vlog.VI(1).Infof("Shutdown: %q [DONE]", eh.name)
- eh.stdin.Close()
+ if eh.stdin != nil {
+ eh.stdin.Close()
+ }
defer eh.sh.Forget(eh)
waitStdout := make(chan struct{})
diff --git a/lib/modules/shell.go b/lib/modules/shell.go
index 569cd7a..071a08e 100644
--- a/lib/modules/shell.go
+++ b/lib/modules/shell.go
@@ -186,14 +186,6 @@
return registry.help(command)
}
-func (sh *Shell) StartExternalCommand(env []string, args ...string) (Handle, error) {
- if len(args) == 0 {
- return nil, errors.New("no arguments specified to StartExternalCommand")
- }
- c := newExecHandleForExternalCommand(args[0])
- return sh.startCommand(c, env, args...)
-}
-
// Start starts the specified command, it returns a Handle which can be
// used for interacting with that command.
//
@@ -235,6 +227,27 @@
return h, nil
}
+// StartExternalCommand starts the specified external, non-Vanadium, command;
+// it returns a Handle which can be used for interacting with that command.
+// A non-Vanadium command does not implement the parent-child protocol
+// implemented by the veyron/lib/exec library, thus this method can be used
+// to start any command (e.g. /bin/cp).
+//
+// StartExternalCommand takes an io.Reader which, if non-nil, will be used as
+// the stdin for the child process. If this parameter is supplied, then the
+// Stdin() method on the returned Handle will return nil. The client of this
+// API maintains ownership of stdin and must close it, i.e. the shell
+// will not do so.
+// The env and args parameters are handled in the same way as Start.
+func (sh *Shell) StartExternalCommand(stdin io.Reader, env []string, args ...string) (Handle, error) {
+ if len(args) == 0 {
+ return nil, errors.New("no arguments specified to StartExternalCommand")
+ }
+ c := newExecHandleForExternalCommand(args[0], stdin)
+ expanded := sh.expand(args...)
+ return sh.startCommand(c, env, expanded...)
+}
+
func (sh *Shell) startCommand(c command, env []string, args ...string) (Handle, error) {
cenv, err := sh.setupCommandEnv(env)
if err != nil {
diff --git a/lib/testutil/v23tests/v23tests.go b/lib/testutil/v23tests/v23tests.go
index 246b514..2b77e43 100644
--- a/lib/testutil/v23tests/v23tests.go
+++ b/lib/testutil/v23tests/v23tests.go
@@ -307,7 +307,7 @@
func (b *Binary) start(skip int, args ...string) *Invocation {
vlog.Infof("%s: starting %s %s", Caller(skip+1), b.Path(), strings.Join(args, " "))
- handle, err := b.env.shell.StartExternalCommand(b.envVars, append([]string{b.Path()}, args...)...)
+ handle, err := b.env.shell.StartExternalCommand(b.inputReader, b.envVars, append([]string{b.Path()}, args...)...)
if err != nil {
// TODO(cnicolaou): calling Fatalf etc from a goroutine often leads
// to deadlock. Need to make sure that we handle this here. Maybe
@@ -325,24 +325,6 @@
Session: expect.NewSession(b.env, handle.Stdout(), 5*time.Minute),
}
b.env.appendInvocation(inv)
- if b.inputReader != nil {
- // This goroutine makes a best-effort attempt to copy bytes
- // from b.inputReader to inv.Stdin() using io.Copy. When Copy
- // returns (successfully or not), inv.CloseStdin() is called.
- // This is always safe, even if inv has been shutdown.
- //
- // This goroutine's lifespan will be limited to that of the
- // environment to which 'inv' is attached. This is because the
- // environment will take care to kill all remaining invocations
- // upon Cleanup, which will in turn cause Copy to fail and
- // therefore this goroutine will exit.
- go func() {
- if _, err := io.Copy(inv.Stdin(), b.inputReader); err != nil {
- vlog.Infof("%s: Copy failed: %v", Caller(skip+2), err)
- }
- inv.CloseStdin()
- }()
- }
return inv
}
diff --git a/lib/testutil/v23tests/v23tests_test.go b/lib/testutil/v23tests/v23tests_test.go
index 88e33e0..4c5b06c 100644
--- a/lib/testutil/v23tests/v23tests_test.go
+++ b/lib/testutil/v23tests/v23tests_test.go
@@ -148,7 +148,7 @@
cat := env.BinaryFromPath("/bin/cat")
if want, got := "Hello, world!\n", cat.WithStdin(echo.Start("Hello, world!").Stdout()).Start().Output(); want != got {
- t.Fatalf("unexpected output, got %s, want %s", got, want)
+ t.Fatalf("unexpected output, got %q, want %q", got, want)
}
// Read something from a file.
@@ -158,7 +158,7 @@
f.WriteString(want)
f.Seek(0, 0)
if got := cat.WithStdin(f).Start().Output(); want != got {
- t.Fatalf("unexpected output, got %s, want %s", got, want)
+ t.Fatalf("unexpected output, got %q, want %q", got, want)
}
}
