From 8e0f891ebcbc66591060b3196906501c0fd6e8f0 Mon Sep 17 00:00:00 2001
From: Li Yuxuan <liyuxuan04@baidu.com>
Date: Tue, 7 Apr 2020 13:13:17 +0800
Subject: [PATCH] v2: Open log fifo with `RDWR` instead of `WRONLY`

The container log fifo is opened as `O_WRONLY` now. When the read side
of fifo is closed temporarily such as restarting contaienrd, write to
`tty.Stdout` will get an EPIPE error and finally cause `io.CopyBuffer`
return. Then `ioCopy` closes the tty io and exits. Thus after containerd
restarted, the log fifo can't be reopened. The container will be blocked
forever after stdout/stderr buffer is full.

Opening the log fifo with `RDWR` instead of `WRONLY` avoids the fifo
returning EPIPE when the read side is closed, and keeps the fifo open
until the reader reopening it.

Fixes: #2590

Signed-off-by: Li Yuxuan <liyuxuan04@baidu.com>
---
 containerd-shim-v2/stream.go      |  4 +-
 containerd-shim-v2/stream_test.go | 91 +++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 containerd-shim-v2/stream_test.go

diff --git a/containerd-shim-v2/stream.go b/containerd-shim-v2/stream.go
index e8c3b5c66f..a16cbba0c4 100644
--- a/containerd-shim-v2/stream.go
+++ b/containerd-shim-v2/stream.go
@@ -63,14 +63,14 @@ func newTtyIO(ctx context.Context, stdin, stdout, stderr string, console bool) (
 	}
 
 	if stdout != "" {
-		outw, err = fifo.OpenFifo(ctx, stdout, syscall.O_WRONLY, 0)
+		outw, err = fifo.OpenFifo(ctx, stdout, syscall.O_RDWR, 0)
 		if err != nil {
 			return nil, err
 		}
 	}
 
 	if !console && stderr != "" {
-		errw, err = fifo.OpenFifo(ctx, stderr, syscall.O_WRONLY, 0)
+		errw, err = fifo.OpenFifo(ctx, stderr, syscall.O_RDWR, 0)
 		if err != nil {
 			return nil, err
 		}
diff --git a/containerd-shim-v2/stream_test.go b/containerd-shim-v2/stream_test.go
new file mode 100644
index 0000000000..637a20b034
--- /dev/null
+++ b/containerd-shim-v2/stream_test.go
@@ -0,0 +1,91 @@
+// Copyright (c) 2020 Baidu Corporation
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+
+package containerdshim
+
+import (
+	"context"
+	"io"
+	"io/ioutil"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/containerd/fifo"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewTtyIOFifoReopen(t *testing.T) {
+	var outr io.ReadWriteCloser
+	var errr io.ReadWriteCloser
+	var tty *ttyIO
+	assert := assert.New(t)
+	ctx := context.TODO()
+	fifoPath, err := ioutil.TempDir(testDir, "fifo-path-")
+	assert.NoError(err)
+	stdout := filepath.Join(fifoPath, "stdout")
+	stderr := filepath.Join(fifoPath, "stderr")
+
+	createReadFifo := func(f string) io.ReadWriteCloser {
+		rf, err := fifo.OpenFifo(ctx, f, syscall.O_RDONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700)
+		if err != nil {
+			t.Fatal(err)
+		}
+		return rf
+	}
+
+	outr = createReadFifo(stdout)
+	defer outr.Close()
+	errr = createReadFifo(stderr)
+	defer errr.Close()
+	tty, err = newTtyIO(ctx, "", stdout, stderr, false)
+	assert.NoError(err)
+	defer tty.close()
+
+	testBytes := []byte("T")
+	checkFifoWrite := func(w io.Writer) {
+		_, err = w.Write(testBytes)
+		assert.NoError(err)
+	}
+	checkFifoRead := func(r io.Reader) {
+		var err error
+		buf := make([]byte, 1)
+		done := make(chan struct{})
+		timer := time.NewTimer(2 * time.Second)
+		go func() {
+			_, err = r.Read(buf)
+			close(done)
+		}()
+		select {
+		case <-done:
+			assert.NoError(err)
+			assert.Equal(buf, testBytes)
+		case <-timer.C:
+			t.Fatal("read fifo timeout")
+		}
+	}
+
+	checkFifoWrite(tty.Stdout)
+	checkFifoRead(outr)
+	checkFifoWrite(tty.Stderr)
+	checkFifoRead(errr)
+
+	err = outr.Close()
+	assert.NoError(err)
+	err = errr.Close()
+	assert.NoError(err)
+
+	// Make sure that writing to tty fifo will not get `EPIPE`
+	// when the read side is closed
+	checkFifoWrite(tty.Stdout)
+	checkFifoWrite(tty.Stderr)
+
+	// Reopen the fifo
+	outr = createReadFifo(stdout)
+	errr = createReadFifo(stderr)
+	checkFifoRead(outr)
+	checkFifoRead(errr)
+}