Merge pull request containerd#58 from sipsma/gorace

Fix data race in use of cmd output buffers.

Author: crosbymichael

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)
 }