archive/tar: bug fixes.

1. If all data is exhausted using Read then a following Next will
   fail as if it saw EOF.  (Test case added.)
2. Seeking isn't always possible (i.e. sockets and pipes).  Fallback
   to read.  (Test case added.)
3. Fix to readHeader (cleaner fix pointed out by rsc).
   (TestReader modified.)
4. When Read has consumed all the data, don't try to read 0 bytes from reader.
   In cases where tr.nb is zero we attempt to read zero bytes and thus
   never see an EOF (this is most easily seen when the 'tar source' is
   something like bytes.Buffer{} as opposed to os.File).
5. If write is used to the point of ErrWriteTooLong, allow additional file entries.
6. Make close work as expected.  That is any further Write or
   WriteHeader attempts will result in ErrWriteAfterClose.
Fixes #419.

R=rsc, dsymonds1
https://golang.org/cl/162062

src/pkg/archive/tar/reader.go

@@ -93,13 +93,13 @@ func (ignoreWriter) Write(b []byte) (n int, err os.Error) {
 // Skip any unread bytes in the existing file entry, as well as any alignment padding.
 func (tr *Reader) skipUnread() {
 	nr := tr.nb + tr.pad;	// number of bytes to skip
-
-	if sr, ok := tr.r.(io.Seeker); ok {
-		_, tr.err = sr.Seek(nr, 1)
-	} else {
-		_, tr.err = io.Copyn(ignoreWriter{}, tr.r, nr)
-	}
 	tr.nb, tr.pad = 0, 0;
+	if sr, ok := tr.r.(io.Seeker); ok {
+		if _, err := sr.Seek(nr, 1); err == nil {
+			return
+		}
+	}
+	_, tr.err = io.Copyn(ignoreWriter{}, tr.r, nr);
 }
 
 func (tr *Reader) verifyChecksum(header []byte) bool {
@@ -123,8 +123,10 @@ func (tr *Reader) readHeader() *Header {
 		if _, tr.err = io.ReadFull(tr.r, header); tr.err != nil {
 			return nil
 		}
-		if !bytes.Equal(header, zeroBlock[0:blockSize]) {
-			tr.err = HeaderError
+		if bytes.Equal(header, zeroBlock[0:blockSize]) {
+			tr.err = os.EOF
+		} else {
+			tr.err = HeaderError	// zero block and then non-zero block
 		}
 		return nil;
 	}
@@ -202,14 +204,23 @@ func (tr *Reader) readHeader() *Header {
 }
 
 // Read reads from the current entry in the tar archive.
-// It returns 0, nil when it reaches the end of that entry,
+// It returns 0, os.EOF when it reaches the end of that entry,
 // until Next is called to advance to the next entry.
 func (tr *Reader) Read(b []byte) (n int, err os.Error) {
+	if tr.nb == 0 {
+		// file consumed
+		return 0, os.EOF
+	}
+
 	if int64(len(b)) > tr.nb {
 		b = b[0:tr.nb]
 	}
 	n, err = tr.r.Read(b);
 	tr.nb -= int64(n);
+
+	if err == os.EOF && tr.nb > 0 {
+		err = io.ErrUnexpectedEOF
+	}
 	tr.err = err;
 	return;
 }

src/pkg/archive/tar/reader_test.go

@@ -6,6 +6,8 @@ package tar
 
 import (
 	"bytes";
+	"crypto/md5";
+	"fmt";
 	"io";
 	"os";
 	"reflect";
@@ -16,36 +18,43 @@ import (
 type untarTest struct {
 	file	string;
 	headers	[]*Header;
+	cksums	[]string;
+}
+
+var gnuTarTest = &untarTest{
+	file: "testdata/gnu.tar",
+	headers: []*Header{
+		&Header{
+			Name: "small.txt",
+			Mode: 0640,
+			Uid: 73025,
+			Gid: 5000,
+			Size: 5,
+			Mtime: 1244428340,
+			Typeflag: '0',
+			Uname: "dsymonds",
+			Gname: "eng",
+		},
+		&Header{
+			Name: "small2.txt",
+			Mode: 0640,
+			Uid: 73025,
+			Gid: 5000,
+			Size: 11,
+			Mtime: 1244436044,
+			Typeflag: '0',
+			Uname: "dsymonds",
+			Gname: "eng",
+		},
+	},
+	cksums: []string{
+		"e38b27eaccb4391bdec553a7f3ae6b2f",
+		"c65bd2e50a56a2138bf1716f2fd56fe9",
+	},
 }
 
 var untarTests = []*untarTest{
-	&untarTest{
-		file: "testdata/gnu.tar",
-		headers: []*Header{
-			&Header{
-				Name: "small.txt",
-				Mode: 0640,
-				Uid: 73025,
-				Gid: 5000,
-				Size: 5,
-				Mtime: 1244428340,
-				Typeflag: '0',
-				Uname: "dsymonds",
-				Gname: "eng",
-			},
-			&Header{
-				Name: "small2.txt",
-				Mode: 0640,
-				Uid: 73025,
-				Gid: 5000,
-				Size: 11,
-				Mtime: 1244436044,
-				Typeflag: '0',
-				Uname: "dsymonds",
-				Gname: "eng",
-			},
-		},
-	},
+	gnuTarTest,
 	&untarTest{
 		file: "testdata/star.tar",
 		headers: []*Header{
@@ -124,6 +133,9 @@ testLoop:
 			}
 		}
 		hdr, err := tr.Next();
+		if err == os.EOF {
+			break
+		}
 		if hdr != nil || err != nil {
 			t.Errorf("test %d: Unexpected entry or error: hdr=%v err=%v", i, err)
 		}
@@ -166,3 +178,98 @@ func TestPartialRead(t *testing.T) {
 		t.Errorf("Contents = %v, want %v", buf, expected)
 	}
 }
+
+
+func TestIncrementalRead(t *testing.T) {
+	test := gnuTarTest;
+	f, err := os.Open(test.file, os.O_RDONLY, 0444);
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	defer f.Close();
+
+	tr := NewReader(f);
+
+	headers := test.headers;
+	cksums := test.cksums;
+	nread := 0;
+
+	// loop over all files
+	for ; ; nread++ {
+		hdr, err := tr.Next();
+		if hdr == nil || err == os.EOF {
+			break
+		}
+
+		// check the header
+		if !reflect.DeepEqual(hdr, headers[nread]) {
+			t.Errorf("Incorrect header:\nhave %+v\nwant %+v",
+				*hdr, headers[nread])
+		}
+
+		// read file contents in little chunks EOF,
+		// checksumming all the way
+		h := md5.New();
+		rdbuf := make([]uint8, 8);
+		for {
+			nr, err := tr.Read(rdbuf);
+			if err == os.EOF {
+				break
+			}
+			if err != nil {
+				t.Errorf("Read: unexpected error %v\n", err);
+				break;
+			}
+			h.Write(rdbuf[0:nr]);
+		}
+		// verify checksum
+		have := fmt.Sprintf("%x", h.Sum());
+		want := cksums[nread];
+		if want != have {
+			t.Errorf("Bad checksum on file %s:\nhave %+v\nwant %+v", hdr.Name, have, want)
+		}
+	}
+	if nread != len(headers) {
+		t.Errorf("Didn't process all files\nexpected: %d\nprocessed %d\n", len(headers), nread)
+	}
+}
+
+func TestNonSeekable(t *testing.T) {
+	test := gnuTarTest;
+	f, err := os.Open(test.file, os.O_RDONLY, 0444);
+	if err != nil {
+		t.Fatalf("Unexpected error: %v", err)
+	}
+	defer f.Close();
+
+	// pipe the data in
+	r, w, err := os.Pipe();
+	if err != nil {
+		t.Fatalf("Unexpected error %s", err)
+	}
+	go func() {
+		rdbuf := make([]uint8, 1<<16);
+		for {
+			nr, err := f.Read(rdbuf);
+			w.Write(rdbuf[0:nr]);
+			if err == os.EOF {
+				break
+			}
+		}
+		w.Close();
+	}();
+
+	tr := NewReader(r);
+	nread := 0;
+
+	for ; ; nread++ {
+		hdr, err := tr.Next();
+		if hdr == nil || err == os.EOF {
+			break
+		}
+	}
+
+	if nread != len(test.headers) {
+		t.Errorf("Didn't process all files\nexpected: %d\nprocessed %d\n", len(test.headers), nread)
+	}
+}

src/pkg/archive/tar/writer.go

@@ -15,8 +15,9 @@ import (
 )
 
 var (
-	ErrWriteTooLong	= os.NewError("write too long");
-	ErrFieldTooLong	= os.NewError("header field too long");
+	ErrWriteTooLong		= os.NewError("write too long");
+	ErrFieldTooLong		= os.NewError("header field too long");
+	ErrWriteAfterClose	= os.NewError("write after close");
 )
 
 // A Writer provides sequential writing of a tar archive in POSIX.1 format.
@@ -108,7 +109,11 @@ func (tw *Writer) numeric(b []byte, x int64) {
 
 // WriteHeader writes hdr and prepares to accept the file's contents.
 // WriteHeader calls Flush if it is not the first header.
+// Calling after a Close will return ErrWriteAfterClose.
 func (tw *Writer) WriteHeader(hdr *Header) os.Error {
+	if tw.closed {
+		return ErrWriteAfterClose
+	}
 	if tw.err == nil {
 		tw.Flush()
 	}
@@ -164,6 +169,10 @@ func (tw *Writer) WriteHeader(hdr *Header) os.Error {
 // Write returns the error ErrWriteTooLong if more than
 // hdr.Size bytes are written after WriteHeader.
 func (tw *Writer) Write(b []byte) (n int, err os.Error) {
+	if tw.closed {
+		err = ErrWriteTooLong;
+		return;
+	}
 	overwrite := false;
 	if int64(len(b)) > tw.nb {
 		b = b[0:tw.nb];
@@ -172,12 +181,15 @@ func (tw *Writer) Write(b []byte) (n int, err os.Error) {
 	n, err = tw.w.Write(b);
 	tw.nb -= int64(n);
 	if err == nil && overwrite {
-		err = ErrWriteTooLong
+		err = ErrWriteTooLong;
+		return;
 	}
 	tw.err = err;
 	return;
 }
 
+// Close closes the tar archive, flushing any unwritten
+// data to the underlying writer.
 func (tw *Writer) Close() os.Error {
 	if tw.err != nil || tw.closed {
 		return tw.err