From 6a2d3990d1db676935ea7d1d2f8e2b952998b5d9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 29 Jan 2021 12:09:50 +0100 Subject: [PATCH 1/4] cio: FIFOSet.Close() check if FIFOSet is nill to prevent NPE Signed-off-by: Sebastiaan van Stijn --- cio/io.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cio/io.go b/cio/io.go index 003c6632e..8ee13edda 100644 --- a/cio/io.go +++ b/cio/io.go @@ -80,7 +80,7 @@ type FIFOSet struct { // Close the FIFOSet func (f *FIFOSet) Close() error { - if f.close != nil { + if f != nil && f.close != nil { return f.close() } return nil From baf6c1d5e2563544d9fddfde879486816cc6381b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 29 Jan 2021 12:17:35 +0100 Subject: [PATCH 2/4] cio: openFifos() use named return variables to use in defer() This change is mostly defensive; when checking for the returned error, it's easy to make a mistake, and check for a "local" error, not the actual returned error. This patch changes the function to use a named return variable, which is checked in the defer. Signed-off-by: Sebastiaan van Stijn --- cio/io_unix.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/cio/io_unix.go b/cio/io_unix.go index a92d92978..8b600673f 100644 --- a/cio/io_unix.go +++ b/cio/io_unix.go @@ -103,38 +103,36 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (*cio, error) { }, nil } -func openFifos(ctx context.Context, fifos *FIFOSet) (pipes, error) { - var err error +func openFifos(ctx context.Context, fifos *FIFOSet) (f pipes, retErr error) { defer func() { - if err != nil { + if retErr != nil { fifos.Close() } }() - var f pipes if fifos.Stdin != "" { - if f.Stdin, err = fifo.OpenFifo(ctx, fifos.Stdin, syscall.O_WRONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700); err != nil { - return f, errors.Wrapf(err, "failed to open stdin fifo") + if f.Stdin, retErr = fifo.OpenFifo(ctx, fifos.Stdin, syscall.O_WRONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700); retErr != nil { + return f, errors.Wrapf(retErr, "failed to open stdin fifo") } defer func() { - if err != nil && f.Stdin != nil { + if retErr != nil && f.Stdin != nil { f.Stdin.Close() } }() } if fifos.Stdout != "" { - if f.Stdout, err = fifo.OpenFifo(ctx, fifos.Stdout, syscall.O_RDONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700); err != nil { - return f, errors.Wrapf(err, "failed to open stdout fifo") + if f.Stdout, retErr = fifo.OpenFifo(ctx, fifos.Stdout, syscall.O_RDONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700); retErr != nil { + return f, errors.Wrapf(retErr, "failed to open stdout fifo") } defer func() { - if err != nil && f.Stdout != nil { + if retErr != nil && f.Stdout != nil { f.Stdout.Close() } }() } if !fifos.Terminal && fifos.Stderr != "" { - if f.Stderr, err = fifo.OpenFifo(ctx, fifos.Stderr, syscall.O_RDONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700); err != nil { - return f, errors.Wrapf(err, "failed to open stderr fifo") + if f.Stderr, retErr = fifo.OpenFifo(ctx, fifos.Stderr, syscall.O_RDONLY|syscall.O_CREAT|syscall.O_NONBLOCK, 0700); retErr != nil { + return f, errors.Wrapf(retErr, "failed to open stderr fifo") } } return f, nil From 219fa3d0a5a1d3a86479f6d54aef2b94c1fdf9ea Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 29 Jan 2021 12:31:46 +0100 Subject: [PATCH 3/4] cio.copyIO: fix pipes potentially not being closed (Windows) The defer functions were checking the local variable, and would therefore not be executed, as the function returned if an error occurred. Perhaps best illustrated when renaming the local variables; if fifos.Stdin != "" { l, err1 := winio.ListenPipe(fifos.Stdin, nil) if err1 != nil { return nil, errors.Wrapf(err1, "failed to create stdin pipe %s", fifos.Stdin) } defer func(l net.Listener) { if err1 != nil { l.Close() } }(l) // ... } if fifos.Stdout != "" { l, err2 := winio.ListenPipe(fifos.Stdout, nil) if err2 != nil { return nil, errors.Wrapf(err2, "failed to create stdout pipe %s", fifos.Stdout) } defer func(l net.Listener) { if err2 != nil { l.Close() } }(l) // .... } This patch changes the function to use a named return variable, and to use a single `defer()` that closes all pipes. Signed-off-by: Sebastiaan van Stijn --- cio/io_windows.go | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/cio/io_windows.go b/cio/io_windows.go index ca6cd54a4..5940b4854 100644 --- a/cio/io_windows.go +++ b/cio/io_windows.go @@ -20,7 +20,6 @@ import ( "context" "fmt" "io" - "net" winio "github.com/Microsoft/go-winio" "github.com/containerd/containerd/log" @@ -43,21 +42,28 @@ func NewFIFOSetInDir(_, id string, terminal bool) (*FIFOSet, error) { }, nil), nil } -func copyIO(fifos *FIFOSet, ioset *Streams) (*cio, error) { +func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { var ( set []io.Closer ) + defer func() { + if retErr == nil { + return + } + for _, closer := range set { + if closer == nil { + continue + } + _ = closer.Close() + } + }() + if fifos.Stdin != "" { l, err := winio.ListenPipe(fifos.Stdin, nil) if err != nil { return nil, errors.Wrapf(err, "failed to create stdin pipe %s", fifos.Stdin) } - defer func(l net.Listener) { - if err != nil { - l.Close() - } - }(l) set = append(set, l) go func() { @@ -81,11 +87,6 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (*cio, error) { if err != nil { return nil, errors.Wrapf(err, "failed to create stdout pipe %s", fifos.Stdout) } - defer func(l net.Listener) { - if err != nil { - l.Close() - } - }(l) set = append(set, l) go func() { @@ -109,11 +110,6 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (*cio, error) { if err != nil { return nil, errors.Wrapf(err, "failed to create stderr pipe %s", fifos.Stderr) } - defer func(l net.Listener) { - if err != nil { - l.Close() - } - }(l) set = append(set, l) go func() { From 7a468a3f3f00120c312790b0f49f3e910c144235 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 29 Jan 2021 12:36:04 +0100 Subject: [PATCH 4/4] cio.copyIO: refactor to use cio.Close() (windows) Use the existing `.Close()` method instead of implementing the same logic in this function. The defer sets `cios` to `nil` if an error occurred to preserve the existing behavior. Signed-off-by: Sebastiaan van Stijn --- cio/io_windows.go | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/cio/io_windows.go b/cio/io_windows.go index 5940b4854..ded475788 100644 --- a/cio/io_windows.go +++ b/cio/io_windows.go @@ -43,19 +43,11 @@ func NewFIFOSetInDir(_, id string, terminal bool) (*FIFOSet, error) { } func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { - var ( - set []io.Closer - ) + cios := &cio{config: fifos.Config} defer func() { - if retErr == nil { - return - } - for _, closer := range set { - if closer == nil { - continue - } - _ = closer.Close() + if retErr != nil { + _ = cios.Close() } }() @@ -64,7 +56,7 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { if err != nil { return nil, errors.Wrapf(err, "failed to create stdin pipe %s", fifos.Stdin) } - set = append(set, l) + cios.closers = append(cios.closers, l) go func() { c, err := l.Accept() @@ -87,7 +79,7 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { if err != nil { return nil, errors.Wrapf(err, "failed to create stdout pipe %s", fifos.Stdout) } - set = append(set, l) + cios.closers = append(cios.closers, l) go func() { c, err := l.Accept() @@ -110,7 +102,7 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { if err != nil { return nil, errors.Wrapf(err, "failed to create stderr pipe %s", fifos.Stderr) } - set = append(set, l) + cios.closers = append(cios.closers, l) go func() { c, err := l.Accept() @@ -128,7 +120,7 @@ func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { }() } - return &cio{config: fifos.Config, closers: set}, nil + return cios, nil } // NewDirectIO returns an IO implementation that exposes the IO streams as io.ReadCloser