io: add error check on pipe close functions to avoid error overwriting

jmartin82 · bradfitz · commit f45eb9ff3c96 · 2019-08-28T18:35:24.000Z
The current implementation allows multiple calls `Close` and `CloseWithError` in every side of the pipe, as a result, the original error can be overwritten. This CL fixes this behavior adding an error existence check on `atomicError` type and keeping the first error still available. Fixes golang#24283 Change-Id: Iefe8f758aeb775309424365f8177511062514150 GitHub-Last-Rev: b559540 GitHub-Pull-Request: golang#33239 Reviewed-on: https://go-review.googlesource.com/c/go/+/187197 Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com> Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
diff --git a/src/io/pipe.go b/src/io/pipe.go
@@ -10,19 +10,26 @@ package io
 import (
 	"errors"
 	"sync"
-	"sync/atomic"
 )
 
-// atomicError is a type-safe atomic value for errors.
-// We use a struct{ error } to ensure consistent use of a concrete type.
-type atomicError struct{ v atomic.Value }
+// onceError is an object that will only store an error once.
+type onceError struct {
+	sync.Mutex // guards following
+	err        error
+}
 
-func (a *atomicError) Store(err error) {
-	a.v.Store(struct{ error }{err})
+func (a *onceError) Store(err error) {
+	a.Lock()
+	defer a.Unlock()
+	if a.err != nil {
+		return
+	}
+	a.err = err
 }
-func (a *atomicError) Load() error {
-	err, _ := a.v.Load().(struct{ error })
-	return err.error
+func (a *onceError) Load() error {
+	a.Lock()
+	defer a.Unlock()
+	return a.err
 }
 
 // ErrClosedPipe is the error used for read or write operations on a closed pipe.
@@ -36,8 +43,8 @@ type pipe struct {
 
 	once sync.Once // Protects closing done
 	done chan struct{}
-	rerr atomicError
-	werr atomicError
+	rerr onceError
+	werr onceError
 }
 
 func (p *pipe) Read(b []byte) (n int, err error) {
@@ -135,6 +142,9 @@ func (r *PipeReader) Close() error {
 
 // CloseWithError closes the reader; subsequent writes
 // to the write half of the pipe will return the error err.
+//
+// CloseWithError never overwrites the previous error if it exists
+// and always returns nil.
 func (r *PipeReader) CloseWithError(err error) error {
 	return r.p.CloseRead(err)
 }
@@ -163,7 +173,8 @@ func (w *PipeWriter) Close() error {
 // read half of the pipe will return no bytes and the error err,
 // or EOF if err is nil.
 //
-// CloseWithError always returns nil.
+// CloseWithError never overwrites the previous error if it exists
+// and always returns nil.
 func (w *PipeWriter) CloseWithError(err error) error {
 	return w.p.CloseWrite(err)
 }
diff --git a/src/io/pipe_test.go b/src/io/pipe_test.go
@@ -326,8 +326,8 @@ func TestPipeCloseError(t *testing.T) {
 		t.Errorf("Write error: got %T, want testError1", err)
 	}
 	r.CloseWithError(testError2{})
-	if _, err := w.Write(nil); err != (testError2{}) {
-		t.Errorf("Write error: got %T, want testError2", err)
+	if _, err := w.Write(nil); err != (testError1{}) {
+		t.Errorf("Write error: got %T, want testError1", err)
 	}
 
 	r, w = Pipe()
@@ -336,8 +336,8 @@ func TestPipeCloseError(t *testing.T) {
 		t.Errorf("Read error: got %T, want testError1", err)
 	}
 	w.CloseWithError(testError2{})
-	if _, err := r.Read(nil); err != (testError2{}) {
-		t.Errorf("Read error: got %T, want testError2", err)
+	if _, err := r.Read(nil); err != (testError1{}) {
+		t.Errorf("Read error: got %T, want testError1", err)
 	}
 }
 

Original file line number	Diff line number	Diff line change
`@@ -326,8 +326,8 @@ func TestPipeCloseError(t *testing.T) {`
`326`	`326`	`t.Errorf("Write error: got %T, want testError1", err)`
`327`	`327`	`}`
`328`	`328`	`r.CloseWithError(testError2{})`
`329`		`- if _, err := w.Write(nil); err != (testError2{}) {`
`330`		`- t.Errorf("Write error: got %T, want testError2", err)`
	`329`	`+ if _, err := w.Write(nil); err != (testError1{}) {`
	`330`	`+ t.Errorf("Write error: got %T, want testError1", err)`
`331`	`331`	`}`
`332`	`332`
`333`	`333`	`r, w = Pipe()`
`@@ -336,8 +336,8 @@ func TestPipeCloseError(t *testing.T) {`
`336`	`336`	`t.Errorf("Read error: got %T, want testError1", err)`
`337`	`337`	`}`
`338`	`338`	`w.CloseWithError(testError2{})`
`339`		`- if _, err := r.Read(nil); err != (testError2{}) {`
`340`		`- t.Errorf("Read error: got %T, want testError2", err)`
	`339`	`+ if _, err := r.Read(nil); err != (testError1{}) {`
	`340`	`+ t.Errorf("Read error: got %T, want testError1", err)`
`341`	`341`	`}`
`342`	`342`	`}`
`343`	`343`