archive: fix race condition in cmdStream

There is a race condition in pkg/archive when using `cmd.Start` for pigz
and xz where the `*bufio.Reader` could be returned to the pool while the
command is still writing to it, and then picked up and used by a new
command.

The command is wrapped in a `CommandContext` where the process will be
killed when the context is cancelled, however this is not instantaneous,
so there's a brief window while the command is still running but the
`*bufio.Reader` was already returned to the pool.

wrapReadCloser calls `cancel()`, and then `readBuf.Close()` which
eventually returns the buffer to the pool. However, because cmdStream
runs `cmd.Wait` in a go routine that we never wait for to finish, it is
not safe to return the reader to the pool yet.  We need to ensure we
wait for `cmd.Wait` to finish!

Signed-off-by: Stephen Benjamin <stephen@redhat.com>

pkg/archive/archive.go

@@ -1218,6 +1218,9 @@ func cmdStream(cmd *exec.Cmd, input io.Reader) (io.ReadCloser, error) {
 		return nil, err
 	}
 
+	// Ensure the command has exited before we clean anything up
+	done := make(chan struct{})
+
 	// Copy stdout to the returned pipe
 	go func() {
 		if err := cmd.Wait(); err != nil {
@@ -1225,9 +1228,16 @@ func cmdStream(cmd *exec.Cmd, input io.Reader) (io.ReadCloser, error) {
 		} else {
 			pipeW.Close()
 		}
+		close(done)
 	}()
 
-	return pipeR, nil
+	return ioutils.NewReadCloserWrapper(pipeR, func() error {
+		// Close pipeR, and then wait for the command to complete before returning. We have to close pipeR first, as
+		// cmd.Wait waits for any non-file stdout/stderr/stdin to close.
+		err := pipeR.Close()
+		<-done
+		return err
+	}), nil
 }
 
 // NewTempArchive reads the content of src into a temporary file, and returns the contents

pkg/archive/archive_test.go

@@ -1332,7 +1332,9 @@ func TestPigz(t *testing.T) {
 	_, err := exec.LookPath("unpigz")
 	if err == nil {
 		t.Log("Tested whether Pigz is used, as it installed")
-		assert.Equal(t, reflect.TypeOf(contextReaderCloserWrapper.Reader), reflect.TypeOf(&io.PipeReader{}))
+		// For the command wait wrapper
+		cmdWaitCloserWrapper := contextReaderCloserWrapper.Reader.(*ioutils.ReadCloserWrapper)
+		assert.Equal(t, reflect.TypeOf(cmdWaitCloserWrapper.Reader), reflect.TypeOf(&io.PipeReader{}))
 	} else {
 		t.Log("Tested whether Pigz is not used, as it not installed")
 		assert.Equal(t, reflect.TypeOf(contextReaderCloserWrapper.Reader), reflect.TypeOf(&gzip.Reader{}))