code review 4815068: archive/zip: more efficient reader and bug fix

archive/zip: more efficient reader and bug fix

Fixes issue 2090.

[adg, 2011-07-28 23:59:04]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[adg, 2011-07-29 00:07:15]

Please hold off; changed my mnd.

[adg, 2011-07-29 00:23:31]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-07-29 01:46:04]

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:115: // Open returns a ReadCloser that provides
access to the File's contents.
document that Open is safe for concurrent use by multiple goroutines.

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:168: b := make([]byte, fileHeaderLen)
Is it any more efficient (especially once we have escape analysis) to use a:

var buf [fileHeaderLen]byte

?

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:200: b := make([]byte, fileHeaderLen)
likewise

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:208: filenameLen := int(c.Uint16(b[26:28]))
where you also going to check that flags (or something else?) matched the TOC?

[adg, 2011-07-29 07:26:39]

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:115: // Open returns a ReadCloser that provides
access to the File's contents.
On 2011/07/29 01:46:04, bradfitz wrote:
> document that Open is safe for concurrent use by multiple goroutines.

Done.

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:168: b := make([]byte, fileHeaderLen)
Good call.

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:200: b := make([]byte, fileHeaderLen)
On 2011/07/29 01:46:04, bradfitz wrote:
> likewise

Done.

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:208: filenameLen := int(c.Uint16(b[26:28]))
On 2011/07/29 01:46:04, bradfitz wrote:
> where you also going to check that flags (or something else?) matched the TOC?

I thought about it some more, and I don't think it matters.

[bradfitz, 2011-07-29 07:42:42]

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:168: b := make([]byte, fileHeaderLen)
On 2011/07/29 07:26:40, adg wrote:
> Good call.

Actually it doesn't help. The slice gets passed to an interface method inside
io.ReadFull, so all bets are off escape-analysis-wise.

So they're equivalent (iiuc). Pick your prettiest preference.

http://codereview.appspot.com/4815068/diff/11001/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:208: filenameLen := int(c.Uint16(b[26:28]))
On 2011/07/29 07:26:40, adg wrote:
> On 2011/07/29 01:46:04, bradfitz wrote:
> > where you also going to check that flags (or something else?) matched the
TOC?
> 
> I thought about it some more, and I don't think it matters.

I hope you at least appreciated that I threw in a "where you" for you.

[adg, 2011-07-29 08:10:20]

On 29 July 2011 00:42,  <bradfitz@golang.org> wrote:
> Actually it doesn't help. The slice gets passed to an interface method
> inside io.ReadFull, so all bets are off escape-analysis-wise.
>
> So they're equivalent (iiuc). Pick your prettiest preference.

I think the array is still stack-allocated in this case, so it's
probably a win anyway.

[bradfitz, 2011-07-29 16:52:35]

http://codereview.appspot.com/4815068/diff/12004/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

http://codereview.appspot.com/4815068/diff/12004/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:105: return err
is this guaranteed that err != nil here?  Kinda.

This is pretty convoluted.  You have to know the two return types that mean
"end" (kinda) from readDirectoryHeader, which isn't even documented down there,
and you don't distinguish between the io.ReadFull in readDirectoryHeader having
a zero-bytes-read UnexpectedEOF (what you really care about) and an
UnexpectedEOF where there's part of a file header but not all of it (which is
actually a bad error).

I'd check the readDirectoryHeader return type to be:

(ok bool, err os.Error)

And return (false, nil) if zero bytes were read.  Or if you want to keep it just
returning os.Error, document that it returns os.EOF on real end-of-file and any
other error on messed up conditions (including a ReadFull reading half of a
fileheader)

http://codereview.appspot.com/4815068/diff/12004/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:215: if _, err := io.ReadFull(r, b[:]); err != nil
{
This _ value I think we care about.  (zero vs. anything else)

[adg, 2011-07-29 17:17:36]

On 29 July 2011 09:52,  <bradfitz@golang.org> wrote:
>
> http://codereview.appspot.com/4815068/diff/12004/src/pkg/archive/zip/reader.go
> File src/pkg/archive/zip/reader.go (right):
>
>
http://codereview.appspot.com/4815068/diff/12004/src/pkg/archive/zip/reader.g...
> src/pkg/archive/zip/reader.go:105: return err
> is this guaranteed that err != nil here?  Kinda.
>
> This is pretty convoluted.  You have to know the two return types that
> mean "end" (kinda) from readDirectoryHeader, which isn't even documented
> down there, and you don't distinguish between the io.ReadFull in
> readDirectoryHeader having a zero-bytes-read UnexpectedEOF (what you
> really care about) and an UnexpectedEOF where there's part of a file
> header but not all of it (which is actually a bad error).
>
> I'd check the readDirectoryHeader return type to be:
>
> (ok bool, err os.Error)
>
> And return (false, nil) if zero bytes were read.  Or if you want to keep
> it just returning os.Error, document that it returns os.EOF on real
> end-of-file and any other error on messed up conditions (including a
> ReadFull reading half of a fileheader)

Unfortunately, zip files end with a "directory end header", so the
final read won't be zero length but some shorter read.

The check that we've found the correct number of directory will work
in every case except the rare case where a zip has n > 65k files AND
the directory is corrupt at the (n % 65k)th directory entry. Only the
init function knows if it has read the right number of entires;
readDirectoryHeader doesn't have the context to say "ok".

I think the pathological case is rare enough that we can ignore it; at
least, I can't think of an algorithm that works otherwise.

>
http://codereview.appspot.com/4815068/diff/12004/src/pkg/archive/zip/reader.g...
> src/pkg/archive/zip/reader.go:215: if _, err := io.ReadFull(r, b[:]);
> err != nil {
> This _ value I think we care about.  (zero vs. anything else)
>
> http://codereview.appspot.com/4815068/
>

[adg, 2011-07-29 17:24:57]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-07-29 17:25:22]

I added some more comments, ptal.

[bradfitz, 2011-07-29 17:43:54]

LGTM

[adg, 2011-07-29 17:47:04]

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

archive/zip: more efficient reader and bug fix

Fixes issue 2090.

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