Fix data race in use of cmd output buffers.

The bytes.Buffers used to store the output of subprocesses are managed with a
sync.Pool. However, before this change, they were being returned to the Pool
while the slice underlying the Buffer was still in use by caller methods. If
that Buffer was then used by another caller, reads and writes to the underlying
slice had the potential race with one another (which is detectable by running
with the "-race" flag enabled in a binary and/or a test).

This change puts the responsibility on the caller of cmdOutput to put the buffer
back in the pool once it is done reading data from it, ensuring no two methods
use its slice at the same time.

Signed-off-by: Erik Sipsma <[email protected]>

Author: sipsma

runc.go

@@ -17,6 +17,7 @@
 package runc
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -72,11 +73,12 @@ type Runc struct {
 // List returns all containers created inside the provided runc root directory
 func (r *Runc) List(context context.Context) ([]*Container, error) {
 	data, err := cmdOutput(r.command(context, "list", "--format=json"), false)
+	defer putBuf(data)
 	if err != nil {
 		return nil, err
 	}
 	var out []*Container
-	if err := json.Unmarshal(data, &out); err != nil {
+	if err := json.Unmarshal(data.Bytes(), &out); err != nil {
 		return nil, err
 	}
 	return out, nil
@@ -85,11 +87,12 @@ func (r *Runc) List(context context.Context) ([]*Container, error) {
 // State returns the state for the container provided by id
 func (r *Runc) State(context context.Context, id string) (*Container, error) {
 	data, err := cmdOutput(r.command(context, "state", id), true)
+	defer putBuf(data)
 	if err != nil {
-		return nil, fmt.Errorf("%s: %s", err, data)
+		return nil, fmt.Errorf("%s: %s", err, data.String())
 	}
 	var c Container
-	if err := json.Unmarshal(data, &c); err != nil {
+	if err := json.Unmarshal(data.Bytes(), &c); err != nil {
 		return nil, err
 	}
 	return &c, nil
@@ -154,8 +157,9 @@ func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOp
 
 	if cmd.Stdout == nil && cmd.Stderr == nil {
 		data, err := cmdOutput(cmd, true)
+		defer putBuf(data)
 		if err != nil {
-			return fmt.Errorf("%s: %s", err, data)
+			return fmt.Errorf("%s: %s", err, data.String())
 		}
 		return nil
 	}
@@ -233,8 +237,9 @@ func (r *Runc) Exec(context context.Context, id string, spec specs.Process, opts
 	}
 	if cmd.Stdout == nil && cmd.Stderr == nil {
 		data, err := cmdOutput(cmd, true)
+		defer putBuf(data)
 		if err != nil {
-			return fmt.Errorf("%s: %s", err, data)
+			return fmt.Errorf("%s: %s", err, data.String())
 		}
 		return nil
 	}
@@ -399,11 +404,12 @@ func (r *Runc) Resume(context context.Context, id string) error {
 // Ps lists all the processes inside the container returning their pids
 func (r *Runc) Ps(context context.Context, id string) ([]int, error) {
 	data, err := cmdOutput(r.command(context, "ps", "--format", "json", id), true)
+	defer putBuf(data)
 	if err != nil {
-		return nil, fmt.Errorf("%s: %s", err, data)
+		return nil, fmt.Errorf("%s: %s", err, data.String())
 	}
 	var pids []int
-	if err := json.Unmarshal(data, &pids); err != nil {
+	if err := json.Unmarshal(data.Bytes(), &pids); err != nil {
 		return nil, err
 	}
 	return pids, nil
@@ -412,11 +418,12 @@ func (r *Runc) Ps(context context.Context, id string) ([]int, error) {
 // Top lists all the processes inside the container returning the full ps data
 func (r *Runc) Top(context context.Context, id string, psOptions string) (*TopResults, error) {
 	data, err := cmdOutput(r.command(context, "ps", "--format", "table", id, psOptions), true)
+	defer putBuf(data)
 	if err != nil {
-		return nil, fmt.Errorf("%s: %s", err, data)
+		return nil, fmt.Errorf("%s: %s", err, data.String())
 	}
 
-	topResults, err := ParsePSOutput(data)
+	topResults, err := ParsePSOutput(data.Bytes())
 	if err != nil {
 		return nil, fmt.Errorf("%s: ", err)
 	}
@@ -606,10 +613,11 @@ type Version struct {
 // Version returns the runc and runtime-spec versions
 func (r *Runc) Version(context context.Context) (Version, error) {
 	data, err := cmdOutput(r.command(context, "--version"), false)
+	defer putBuf(data)
 	if err != nil {
 		return Version{}, err
 	}
-	return parseVersion(data)
+	return parseVersion(data.Bytes())
 }
 
 func parseVersion(data []byte) (Version, error) {
@@ -687,15 +695,17 @@ func (r *Runc) runOrError(cmd *exec.Cmd) error {
 		return err
 	}
 	data, err := cmdOutput(cmd, true)
+	defer putBuf(data)
 	if err != nil {
-		return fmt.Errorf("%s: %s", err, data)
+		return fmt.Errorf("%s: %s", err, data.String())
 	}
 	return nil
 }
 
-func cmdOutput(cmd *exec.Cmd, combined bool) ([]byte, error) {
+// callers of cmdOutput are expected to call putBuf on the returned Buffer
+// to ensure it is released back to the shared pool after use.
+func cmdOutput(cmd *exec.Cmd, combined bool) (*bytes.Buffer, error) {
 	b := getBuf()
-	defer putBuf(b)
 
 	cmd.Stdout = b
 	if combined {
@@ -711,5 +721,5 @@ func cmdOutput(cmd *exec.Cmd, combined bool) ([]byte, error) {
 		err = fmt.Errorf("%s did not terminate successfully", cmd.Args[0])
 	}
 
-	return b.Bytes(), err
+	return b, err
 }

runc_test.go

@@ -16,7 +16,11 @@
 
 package runc
 
-import "testing"
+import (
+	"context"
+	"sync"
+	"testing"
+)
 
 func TestParseVersion(t *testing.T) {
 	const data = `runc version 1.0.0-rc3
@@ -37,3 +41,25 @@ spec: 1.0.0-rc5-dev`
 		t.Errorf("expected spec version 1.0.0-rc5-dev but received %s", v.Spec)
 	}
 }
+
+func TestParallelCmds(t *testing.T) {
+	rc := &Runc{
+		// we don't need a real runc, we just want to test running a caller of cmdOutput in parallel
+		Command: "/bin/true",
+	}
+	var wg sync.WaitGroup
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	for i := 0; i < 256; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			// We just want to fail if there is a race condition detected by
+			// "-race", so we ignore the (expected) error here.
+			_, _ = rc.Version(ctx)
+		}()
+	}
+	wg.Wait()
+}

utils.go

@@ -57,6 +57,10 @@ func getBuf() *bytes.Buffer {
 }
 
 func putBuf(b *bytes.Buffer) {
+	if b == nil {
+		return
+	}
+
 	b.Reset()
 	bytesBufferPool.Put(b)
 }