From 7246585f8c4211df2b678639ba58a72f70573c3c Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Mon, 25 Sep 2017 15:03:49 -0700 Subject: [PATCH] archive/tar: avoid empty IO operations The interfaces for io.Reader and io.Writer permit calling Read/Write with an empty buffer. However, this condition is often not well tested and can lead to bugs in various implementations of io.Reader and io.Writer. For example, see #22028 for buggy io.Reader in the bzip2 package. We reduce the likelihood of hitting these bugs by adjusting regFileReader.Read and regFileWriter.Write to avoid performing Read and Write calls when the buffer is known to be empty. Fixes #22029 Change-Id: Ie4a26be53cf87bc4d2abd951fa005db5871cc75c Reviewed-on: https://go-review.googlesource.com/66111 Run-TryBot: Joe Tsai Reviewed-by: Giovanni Bajo Reviewed-by: Ian Lance Taylor TryBot-Result: Gobot Gobot --- src/archive/tar/reader.go | 8 +++++--- src/archive/tar/reader_test.go | 17 ++++++++++++++--- src/archive/tar/writer.go | 8 +++++--- src/archive/tar/writer_test.go | 17 +++++++++++++++-- 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/archive/tar/reader.go b/src/archive/tar/reader.go index c2e3041d92..94fa417308 100644 --- a/src/archive/tar/reader.go +++ b/src/archive/tar/reader.go @@ -649,12 +649,14 @@ type regFileReader struct { nb int64 // Number of remaining bytes to read } -func (fr *regFileReader) Read(b []byte) (int, error) { +func (fr *regFileReader) Read(b []byte) (n int, err error) { if int64(len(b)) > fr.nb { b = b[:fr.nb] } - n, err := fr.r.Read(b) - fr.nb -= int64(n) + if len(b) > 0 { + n, err = fr.r.Read(b) + fr.nb -= int64(n) + } switch { case err == io.EOF && fr.nb > 0: return n, io.ErrUnexpectedEOF diff --git a/src/archive/tar/reader_test.go b/src/archive/tar/reader_test.go index 256f0eaca1..bbabd96246 100644 --- a/src/archive/tar/reader_test.go +++ b/src/archive/tar/reader_test.go @@ -7,6 +7,7 @@ package tar import ( "bytes" "crypto/md5" + "errors" "fmt" "io" "io/ioutil" @@ -1395,6 +1396,17 @@ func TestReadGNUSparsePAXHeaders(t *testing.T) { } } +// testNonEmptyReader wraps an io.Reader and ensures that +// Read is never called with an empty buffer. +type testNonEmptyReader struct{ io.Reader } + +func (r testNonEmptyReader) Read(b []byte) (int, error) { + if len(b) == 0 { + return 0, errors.New("unexpected empty Read call") + } + return r.Reader.Read(b) +} + func TestFileReader(t *testing.T) { type ( testRead struct { // Read(cnt) == (wantStr, wantErr) @@ -1443,7 +1455,6 @@ func TestFileReader(t *testing.T) { maker: makeReg{"", 1}, tests: []testFnc{ testRemaining{1, 1}, - testRead{0, "", io.ErrUnexpectedEOF}, testRead{5, "", io.ErrUnexpectedEOF}, testWriteTo{nil, 0, io.ErrUnexpectedEOF}, testRemaining{1, 1}, @@ -1611,14 +1622,14 @@ func TestFileReader(t *testing.T) { var fr fileReader switch maker := v.maker.(type) { case makeReg: - r := strings.NewReader(maker.str) + r := testNonEmptyReader{strings.NewReader(maker.str)} fr = ®FileReader{r, maker.size} case makeSparse: if !validateSparseEntries(maker.spd, maker.size) { t.Fatalf("invalid sparse map: %v", maker.spd) } sph := invertSparseEntries(maker.spd, maker.size) - r := strings.NewReader(maker.makeReg.str) + r := testNonEmptyReader{strings.NewReader(maker.makeReg.str)} fr = ®FileReader{r, maker.makeReg.size} fr = &sparseFileReader{fr, sph, 0} default: diff --git a/src/archive/tar/writer.go b/src/archive/tar/writer.go index 0772d8b206..0ae48b8b23 100644 --- a/src/archive/tar/writer.go +++ b/src/archive/tar/writer.go @@ -452,13 +452,15 @@ type regFileWriter struct { nb int64 // Number of remaining bytes to write } -func (fw *regFileWriter) Write(b []byte) (int, error) { +func (fw *regFileWriter) Write(b []byte) (n int, err error) { overwrite := int64(len(b)) > fw.nb if overwrite { b = b[:fw.nb] } - n, err := fw.w.Write(b) - fw.nb -= int64(n) + if len(b) > 0 { + n, err = fw.w.Write(b) + fw.nb -= int64(n) + } switch { case err != nil: return n, err diff --git a/src/archive/tar/writer_test.go b/src/archive/tar/writer_test.go index 122ec7d3d9..ecac29a39e 100644 --- a/src/archive/tar/writer_test.go +++ b/src/archive/tar/writer_test.go @@ -7,6 +7,7 @@ package tar import ( "bytes" "encoding/hex" + "errors" "io" "io/ioutil" "os" @@ -987,6 +988,17 @@ func TestIssue12594(t *testing.T) { } } +// testNonEmptyWriter wraps an io.Writer and ensures that +// Write is never called with an empty buffer. +type testNonEmptyWriter struct{ io.Writer } + +func (w testNonEmptyWriter) Write(b []byte) (int, error) { + if len(b) == 0 { + return 0, errors.New("unexpected empty Write call") + } + return w.Writer.Write(b) +} + func TestFileWriter(t *testing.T) { type ( testWrite struct { // Write(str) == (wantCnt, wantErr) @@ -1225,17 +1237,18 @@ func TestFileWriter(t *testing.T) { for i, v := range vectors { var wantStr string bb := new(bytes.Buffer) + w := testNonEmptyWriter{bb} var fw fileWriter switch maker := v.maker.(type) { case makeReg: - fw = ®FileWriter{bb, maker.size} + fw = ®FileWriter{w, maker.size} wantStr = maker.wantStr case makeSparse: if !validateSparseEntries(maker.sph, maker.size) { t.Fatalf("invalid sparse map: %v", maker.sph) } spd := invertSparseEntries(maker.sph, maker.size) - fw = ®FileWriter{bb, maker.makeReg.size} + fw = ®FileWriter{w, maker.makeReg.size} fw = &sparseFileWriter{fw, spd, 0} wantStr = maker.makeReg.wantStr default: