code review 5777064: archive/tar: catch short writes. (issue 5777064)

[dsym...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

Description:
archive/tar: catch short writes.

Also make error messages consistent throughout.

Please review this at http://codereview.appspot.com/5777064/

Affected files:
M src/pkg/archive/tar/reader.go
M src/pkg/archive/tar/writer.go
M src/pkg/archive/tar/writer_test.go

Index: src/pkg/archive/tar/reader.go
===================================================================
--- a/src/pkg/archive/tar/reader.go
+++ b/src/pkg/archive/tar/reader.go
@@ -18,7 +18,7 @@
)

var (
- ErrHeader = errors.New("invalid tar header")
+ ErrHeader = errors.New("archive/tar: invalid tar header")
)

// A Reader provides sequential access to the contents of a tar archive.
Index: src/pkg/archive/tar/writer.go
===================================================================
--- a/src/pkg/archive/tar/writer.go
+++ b/src/pkg/archive/tar/writer.go
@@ -5,18 +5,19 @@
package tar

// TODO(dsymonds):
-// - catch more errors (no first header, write after close, etc.)
+// - catch more errors (no first header, etc.)

import (
"errors"
+ "fmt"
"io"
"strconv"
)

var (
- ErrWriteTooLong = errors.New("write too long")
- ErrFieldTooLong = errors.New("header field too long")
- ErrWriteAfterClose = errors.New("write after close")
+ ErrWriteTooLong = errors.New("archive/tar: write too long")
+ ErrFieldTooLong = errors.New("archive/tar: header field too long")
+ ErrWriteAfterClose = errors.New("archive/tar: write after close")
)

// A Writer provides sequential writing of a tar archive in POSIX.1 format.
@@ -48,6 +49,11 @@

// Flush finishes writing the current file (optional).
func (tw *Writer) Flush() error {
+ if tw.nb > 0 {
+ tw.err = fmt.Errorf("archive/tar: missed writing %d bytes", tw.nb)
+ return tw.err
+ }
+
n := tw.nb + tw.pad
for n > 0 && tw.err == nil {
nr := n
@@ -193,6 +199,9 @@
}
tw.Flush()
tw.closed = true
+ if tw.err != nil {
+ return tw.err
+ }

// trailer: two zero blocks
for i := 0; i < 2; i++ {
Index: src/pkg/archive/tar/writer_test.go
===================================================================
--- a/src/pkg/archive/tar/writer_test.go
+++ b/src/pkg/archive/tar/writer_test.go
@@ -9,6 +9,7 @@
"fmt"
"io"
"io/ioutil"
+ "strings"
"testing"
"testing/iotest"
"time"
@@ -95,7 +96,8 @@
Uname: "dsymonds",
Gname: "eng",
},
- // no contents
+ // fake contents
+ contents: strings.Repeat("\x00", 4<<10),
},
},
},
@@ -150,7 +152,10 @@

buf := new(bytes.Buffer)
tw := NewWriter(iotest.TruncateWriter(buf, 4<<10)) // only catch the
first 4 KB
+ big := false
for j, entry := range test.entries {
+ big = big || entry.header.Size > 1<<10
+ t.Logf("big: %v", big)
if err := tw.WriteHeader(entry.header); err != nil {
t.Errorf("test %d, entry %d: Failed writing header: %v", i, j, err)
continue testLoop
@@ -160,7 +165,8 @@
continue testLoop
}
}
- if err := tw.Close(); err != nil {
+ // Only interested in Close failures for the small tests.
+ if err := tw.Close(); err != nil && !big {
t.Errorf("test %d: Failed closing archive: %v", i, err)
continue testLoop
}

[brad...@golang.org]

LGTM

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go#newcode53
src/pkg/archive/tar/writer.go:53: tw.err = fmt.Errorf("archive/tar:

missed writing %d bytes", tw.nb)

"I wouldn't say I've been missing it, Bob."

This error message sounds a bit weird. I might say something more like:

"Header declared %d bytes, but only %d bytes were written."

Also, is a Write after a Flush an error?

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go#newcode200
src/pkg/archive/tar/writer.go:200: tw.Flush()
might as well just look at the return value of Flush here, rather than
hoping that Flush set tw.err

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_test.go
File src/pkg/archive/tar/writer_test.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_test.go#newcode158
src/pkg/archive/tar/writer_test.go:158: t.Logf("big: %v", big)
leftover debugging?

http://codereview.appspot.com/5777064/

[dsym...@golang.org]

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go#newcode53
src/pkg/archive/tar/writer.go:53: tw.err = fmt.Errorf("archive/tar:
missed writing %d bytes", tw.nb)

On 2012/03/12 06:27:08, bradfitz wrote:
> "I wouldn't say I've been missing it, Bob."

"I have eight different bosses, Bob!"

> This error message sounds a bit weird. I might say something more
like:

> "Header declared %d bytes, but only %d bytes were written."

Yeah, except we aren't tracking the length of the file. I could add
that, but that's a bigger change than I really wanted to do at this
stage. I'll get the fix in now, and we can wordsmith later.

> Also, is a Write after a Flush an error?

Not necessarily. An empty write is fine, for one.

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer.go#newcode200
src/pkg/archive/tar/writer.go:200: tw.Flush()

On 2012/03/12 06:27:08, bradfitz wrote:
> might as well just look at the return value of Flush here, rather than
hoping
> that Flush set tw.err

Other parts already expect flush to set tw.err, and it's simpler to keep
this consistent.

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_test.go
File src/pkg/archive/tar/writer_test.go (right):

http://codereview.appspot.com/5777064/diff/2004/src/pkg/archive/tar/writer_test.go#newcode158
src/pkg/archive/tar/writer_test.go:158: t.Logf("big: %v", big)

On 2012/03/12 06:27:08, bradfitz wrote:
> leftover debugging?

oops, yes.

http://codereview.appspot.com/5777064/

[dsym...@golang.org]

*** Submitted as
http://code.google.com/p/go/source/detail?r=7dac9470d5d8 ***

archive/tar: catch short writes.

Also make error messages consistent throughout.

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5777064

http://codereview.appspot.com/5777064/

[r...@golang.org]

http://codereview.appspot.com/5777064/diff/5001/src/pkg/archive/tar/writer.go
File src/pkg/archive/tar/writer.go (right):

http://codereview.appspot.com/5777064/diff/5001/src/pkg/archive/tar/writer.go#newcode53

src/pkg/archive/tar/writer.go:53: tw.err = fmt.Errorf("archive/tar:
missed writing %d bytes", tw.nb)

This seems like a regression. The code below was already correctly
handling writing zeros if the written data was too short. Now you are
creating a usage error. This is an API change. The old code seems like
it was fine.

http://codereview.appspot.com/5777064/