cio.copyIO: fix pipes potentially not being closed (Windows)

thaJeztah · thaJeztah · commit 219fa3d0a5a1 · 2021-02-01T11:19:50.000+01:00
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 &lt;github@gone.nl&gt;
diff --git 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() {
