From 6679a5faeb724f1ad060f2fdf6d189f1005924b9 Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Wed, 10 Oct 2018 14:54:00 +0800 Subject: [PATCH 1/2] bugfix: wait for stdin creation before CloseIO The stdin fifo of exec process is created in containerd side after client calls Start. If the client calls CloseIO before Start call, the stdin of exec process is still opened and wait for close. For this case, client closes stdinCloseSync channel after Start. Signed-off-by: Wei Fu (cherry picked from commit c7890f25a9eaae8d07614bd85b2b3231b03e54ec) Signed-off-by: Sebastiaan van Stijn --- libcontainerd/client_daemon.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/libcontainerd/client_daemon.go b/libcontainerd/client_daemon.go index 60fb3353c1..cb9cb43a73 100644 --- a/libcontainerd/client_daemon.go +++ b/libcontainerd/client_daemon.go @@ -328,6 +328,13 @@ func (c *client) Start(ctx context.Context, id, checkpointDir string, withStdin return int(t.Pid()), nil } +// Exec creates exec process. +// +// The containerd client calls Exec to register the exec config in the shim side. +// When the client calls Start, the shim will create stdin fifo if needs. But +// for the container main process, the stdin fifo will be created in Create not +// the Start call. stdinCloseSync channel should be closed after Start exec +// process. func (c *client) Exec(ctx context.Context, containerID, processID string, spec *specs.Process, withStdin bool, attachStdio StdioCallback) (int, error) { ctr := c.getContainer(containerID) if ctr == nil { @@ -372,7 +379,9 @@ func (c *client) Exec(ctx context.Context, containerID, processID string, spec * ctr.addProcess(processID, p) // Signal c.createIO that it can call CloseIO - close(stdinCloseSync) + // + // the stdin of exec process will be created after p.Start in containerd + defer close(stdinCloseSync) if err = p.Start(ctx); err != nil { p.Delete(context.Background()) From ae6284a623bac86ac6ab718fa4a369dd8c0a3cfc Mon Sep 17 00:00:00 2001 From: Wei Fu Date: Thu, 11 Oct 2018 19:03:02 +0800 Subject: [PATCH 2/2] testing: add case for exec closeStdin add regression case for the issue#37870 Signed-off-by: Wei Fu (cherry picked from commit 8e25f4ff6d89888a1bcd578f3f8f7aab89dce24d) Signed-off-by: Sebastiaan van Stijn --- integration/container/exec_test.go | 69 ++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/integration/container/exec_test.go b/integration/container/exec_test.go index 85f9e05915..d33301b8e3 100644 --- a/integration/container/exec_test.go +++ b/integration/container/exec_test.go @@ -4,6 +4,7 @@ import ( "context" "io/ioutil" "testing" + "time" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/strslice" @@ -15,6 +16,74 @@ import ( "gotest.tools/skip" ) +// TestExecWithCloseStdin adds case for moby#37870 issue. +func TestExecWithCloseStdin(t *testing.T) { + skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.39"), "broken in earlier versions") + defer setupTest(t)() + + ctx := context.Background() + client := request.NewAPIClient(t) + + // run top with detached mode + cID := container.Run(t, ctx, client) + + expected := "closeIO" + execResp, err := client.ContainerExecCreate(ctx, cID, + types.ExecConfig{ + AttachStdin: true, + AttachStdout: true, + Cmd: strslice.StrSlice([]string{"sh", "-c", "cat && echo " + expected}), + }, + ) + assert.NilError(t, err) + + resp, err := client.ContainerExecAttach(ctx, execResp.ID, + types.ExecStartCheck{ + Detach: false, + Tty: false, + }, + ) + assert.NilError(t, err) + defer resp.Close() + + // close stdin to send EOF to cat + assert.NilError(t, resp.CloseWrite()) + + var ( + waitCh = make(chan struct{}) + resCh = make(chan struct { + content string + err error + }) + ) + + go func() { + close(waitCh) + defer close(resCh) + r, err := ioutil.ReadAll(resp.Reader) + + resCh <- struct { + content string + err error + }{ + content: string(r), + err: err, + } + }() + + <-waitCh + select { + case <-time.After(3 * time.Second): + t.Fatal("failed to read the content in time") + case got := <-resCh: + assert.NilError(t, got.err) + + // NOTE: using Contains because no-tty's stream contains UX information + // like size, stream type. + assert.Assert(t, is.Contains(got.content, expected)) + } +} + func TestExec(t *testing.T) { skip.If(t, versions.LessThan(testEnv.DaemonAPIVersion(), "1.35"), "broken in earlier versions") defer setupTest(t)()