code review 5787062: archive/zip: write data descriptor signature for OS X; ...

archive/zip: write data descriptor signature for OS X; fix bugs reading it

We now always write the "optional" streaming data descriptor
signature, which turns out to be required for OS X.

Also, handle reading the data descriptor with or without the
signature, per the spec's recommendation. Fix data descriptor
reading bugs found in the process.

Fixes issue 3252

[bradfitz, 2012-03-09 04:29:23]

Hello golang-dev@googlegroups.com,

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

[brainman, 2012-03-09 04:46:03]

Still works on windows.

Alex

[nigeltao, 2012-03-09 12:16:40]

I don't know the zip code or format well enough, but a drive-by comment...

http://codereview.appspot.com/5787062/diff/4001/src/pkg/archive/zip/reader_te...
File src/pkg/archive/zip/reader_test.go (right):

http://codereview.appspot.com/5787062/diff/4001/src/pkg/archive/zip/reader_te...
src/pkg/archive/zip/reader_test.go:20: Source  func() (r io.ReaderAt, size
int64) // if non-nil, used instead of testdata/<Name> file
Would it be easier if Source returned (*Reader, err)?

[bradfitz, 2012-03-09 16:34:23]

On Fri, Mar 9, 2012 at 4:16 AM, <nigeltao@golang.org> wrote:

> I don't know the zip code or format well enough, but a drive-by
> comment...
>
>
> http://codereview.appspot.com/**5787062/diff/4001/src/pkg/**
>
archive/zip/reader_test.go<http://codereview.appspot.com/5787062/diff/4001/src/pkg/archive/zip/reader_test.go>
> File src/pkg/archive/zip/reader_**test.go (right):
>
> http://codereview.appspot.com/**5787062/diff/4001/src/pkg/**
>
archive/zip/reader_test.go#**newcode20<http://codereview.appspot.com/5787062/diff/4001/src/pkg/archive/zip/reader_test.go#newcode20>
> src/pkg/archive/zip/reader_**test.go:20: Source  func() (r io.ReaderAt,
> size int64) // if non-nil, used instead of testdata/<Name> file
> Would it be easier if Source returned (*Reader, err)?
>

Perhaps.  I was going down a different path at the time and switched the
types around a couple times.  If people feel strongly I can switch it, but
it's just test code.

[rsc, 2012-03-09 21:53:15]

LGTM

I can't believe they broke this.

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

http://codereview.appspot.com/5787062/diff/8001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:153: n, err = r.rc.Read(b)
if r.err != nil {
    return 0, r.err
}
n, err = r.rc.Read(b)
if err == nil {
    return
}

Then you won't need r.desr = nil below, and also it will take care of other
errors.

http://codereview.appspot.com/5787062/diff/8001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:158: if err = readDataDescriptor(desr, r.f); err
!= nil {
How about

if err1 := readDataDescriptor(desr, r.f); err1 != nil {
   err = err1
} else if r.hash.Sum32() != r.f.CRC32 {
   err = ErrChecksum
}

Then you don't have to put the EOF back.

http://codereview.appspot.com/5787062/diff/8001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:167: return
r.err = err
return

[bradfitz, 2012-03-09 22:03:36]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com, nigeltao@golang.org,
rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2012-03-09 22:03:37]

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

http://codereview.appspot.com/5787062/diff/8001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:153: n, err = r.rc.Read(b)
On 2012/03/09 21:53:15, rsc wrote:
> if r.err != nil {
>     return 0, r.err
> }
> n, err = r.rc.Read(b)
> if err == nil {
>     return
> }

sure, with the hash.Write in there too

> Then you won't need r.desr = nil below, and also it will take care of other
> errors.

http://codereview.appspot.com/5787062/diff/8001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:158: if err = readDataDescriptor(desr, r.f); err
!= nil {
On 2012/03/09 21:53:15, rsc wrote:
> How about
> 
> if err1 := readDataDescriptor(desr, r.f); err1 != nil {
>    err = err1
> } else if r.hash.Sum32() != r.f.CRC32 {
>    err = ErrChecksum
> }
> 
> Then you don't have to put the EOF back.

Done.

http://codereview.appspot.com/5787062/diff/8001/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:167: return
On 2012/03/09 21:53:15, rsc wrote:
> r.err = err
> return

Done.

[bradfitz, 2012-03-09 22:12:04]

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

archive/zip: write data descriptor signature for OS X; fix bugs reading it

We now always write the "optional" streaming data descriptor
signature, which turns out to be required for OS X.

Also, handle reading the data descriptor with or without the
signature, per the spec's recommendation. Fix data descriptor
reading bugs found in the process.

Fixes issue 3252

R=golang-dev, alex.brainman, nigeltao, rsc
CC=golang-dev
http://codereview.appspot.com/5787062

[adg1, 2012-03-12 01:18:41]

Thanks for tracking this down, Brad. What a crazy world the zip format is...

On Saturday, 10 March 2012, wrote:

> *** Submitted as
>
http://code.google.com/p/go/**source/detail?r=0a07d08fb35b<http://code.google...
>
> archive/zip: write data descriptor signature for OS X; fix bugs reading
> it
>
> We now always write the "optional" streaming data descriptor
> signature, which turns out to be required for OS X.
>
> Also, handle reading the data descriptor with or without the
> signature, per the spec's recommendation. Fix data descriptor
> reading bugs found in the process.
>
> Fixes issue 3252
>
> R=golang-dev, alex.brainman, nigeltao, rsc
> CC=golang-dev
> http://codereview.appspot.com/**5787062<http://codereview.appspot.com/5787062>
>
>
>
http://codereview.appspot.com/**5787062/<http://codereview.appspot.com/5787062/>
>