code review 6811048: archive/zip: handle corrupt extra data records

archive/zip: handle corrupt extra data records

Fixes issue 4302.

[dfc, 2012-10-29 21:47:28]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2012-10-29 21:52:44]

https://codereview.appspot.com/6811048/diff/1002/src/pkg/archive/zip/zip_test.go
File src/pkg/archive/zip/zip_test.go (right):

https://codereview.appspot.com/6811048/diff/1002/src/pkg/archive/zip/zip_test...
src/pkg/archive/zip/zip_test.go:182: func() {
what the func?

https://codereview.appspot.com/6811048/diff/1002/src/pkg/archive/zip/zip_test...
src/pkg/archive/zip/zip_test.go:204: }()
why?

[dfc, 2012-10-29 21:54:30]

https://codereview.appspot.com/6811048/diff/1002/src/pkg/archive/zip/zip_test.go
File src/pkg/archive/zip/zip_test.go (right):

https://codereview.appspot.com/6811048/diff/1002/src/pkg/archive/zip/zip_test...
src/pkg/archive/zip/zip_test.go:182: func() {
On 2012/10/29 21:52:44, bradfitz wrote:
> what the func?

Sorry, this was part of the original failing sample, but makes no sense in this
context.

https://codereview.appspot.com/6811048/diff/1002/src/pkg/archive/zip/zip_test...
src/pkg/archive/zip/zip_test.go:204: }()
On 2012/10/29 21:52:44, bradfitz wrote:
> why?

Done.

[dfc, 2012-10-29 21:54:52]

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

Please take another look.

[bradfitz, 2012-10-29 21:58:52]

https://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

https://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/reader.g...
src/pkg/archive/zip/reader.go:23: ErrCorruptExtra = errors.New("zip: corrupt
extra data")
is this common enough to warrant its own error type variable?  the first two
seem like yes. the checksum one, maybe. this one is starting to really stretch
it.  I'd prefer to make this lowercase for now, and export it later if anybody
actually cares.

https://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/zip_test.go
File src/pkg/archive/zip/zip_test.go (right):

https://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/zip_test...
src/pkg/archive/zip/zip_test.go:183: defer z.Close()
don't you have to do this after fh.Write and before buf.Bytes to ensure a flush
happens?  and check the return value from it.

[dfc, 2012-10-30 13:20:13]

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

Please take another look.

[bradfitz, 2012-10-30 13:42:20]

LGTM

https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/zip_tes...
File src/pkg/archive/zip/zip_test.go (right):

https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/zip_tes...
src/pkg/archive/zip/zip_test.go:206: // TODO(dfc) revisit this when bytes.Buffer
implements ReadAt.
delete this line.  It both makes no sense (to me at least) and also it implies
that bytes.Buffer will one day support ReadAt.  It won't.

[dfc, 2012-10-30 13:46:19]

> src/pkg/archive/zip/zip_test.go:206: // TODO(dfc) revisit this when
> bytes.Buffer implements ReadAt.
> delete this line.  It both makes no sense (to me at least) and also it
> implies that bytes.Buffer will one day support ReadAt.  It won't.

My mistake, I thought that there was a CL in progress to add this.

[bradfitz, 2012-10-30 13:52:33]

On Tue, Oct 30, 2012 at 2:46 PM, Dave Cheney <dave@cheney.net> wrote:

> > src/pkg/archive/zip/zip_test.go:206: // TODO(dfc) revisit this when
> > bytes.Buffer implements ReadAt.
> > delete this line.  It both makes no sense (to me at least) and also it
> > implies that bytes.Buffer will one day support ReadAt.  It won't.
>
> My mistake, I thought that there was a CL in progress to add this.
>

Some ReaderFrom additions went in (and WriterTo?), but not ReaderAt.

A *bytes.Buffer could never implement ReaderAt because the ReaderAt
contract says it's not allowed to be affected by a Seek or Read call, yet a
Read call on a *Buffer destroys the read data and shifts around the "seek
offset" (if it had one).

[adg, 2012-10-30 13:56:26]

NOT LGTM

https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader.go
File src/pkg/archive/zip/reader.go (right):

https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader....
src/pkg/archive/zip/reader.go:23: errCorruptExtra = errors.New("zip: corrupt
extra data")
I don't see a need for this extra error. It should just be ErrFormat.

[bradfitz, 2012-10-30 13:57:38]

On Tue, Oct 30, 2012 at 2:56 PM, <adg@golang.org> wrote:

> NOT LGTM
>
>
> https://codereview.appspot.**com/6811048/diff/12002/src/**
>
pkg/archive/zip/reader.go<https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader.go>
> File src/pkg/archive/zip/reader.go (right):
>
> https://codereview.appspot.**com/6811048/diff/12002/src/**
>
pkg/archive/zip/reader.go#**newcode23<https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader.go#newcode23>
>
> src/pkg/archive/zip/reader.go:**23: errCorruptExtra = errors.New("zip:
> corrupt extra data")
> I don't see a need for this extra error. It should just be ErrFormat.
>

it's lowercase, not an API addition.

[adg, 2012-10-30 14:01:26]

On 31 October 2012 00:57, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Tue, Oct 30, 2012 at 2:56 PM, <adg@golang.org> wrote:
>>
>> NOT LGTM
>>
>>
>>
>>
https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader.go
>> File src/pkg/archive/zip/reader.go (right):
>>
>>
>>
https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader....
>>
>> src/pkg/archive/zip/reader.go:23: errCorruptExtra = errors.New("zip:
>> corrupt extra data")
>> I don't see a need for this extra error. It should just be ErrFormat.
>
>
> it's lowercase, not an API addition.

I saw that, and the discussion, but I think it should just be
ErrFormat. That's what you get for a malformed zip file. There's no
need to return anything else.

[dfc, 2012-10-30 14:28:26]

Thank you for your comments. PTAL.

On Wed, Oct 31, 2012 at 1:00 AM, Andrew Gerrand <adg@golang.org> wrote:
> On 31 October 2012 00:57, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> On Tue, Oct 30, 2012 at 2:56 PM, <adg@golang.org> wrote:
>>>
>>> NOT LGTM
>>>
>>>
>>>
>>>
https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader.go
>>> File src/pkg/archive/zip/reader.go (right):
>>>
>>>
>>>
https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/reader....
>>>
>>> src/pkg/archive/zip/reader.go:23: errCorruptExtra = errors.New("zip:
>>> corrupt extra data")
>>> I don't see a need for this extra error. It should just be ErrFormat.
>>
>>
>> it's lowercase, not an API addition.
>
> I saw that, and the discussion, but I think it should just be
> ErrFormat. That's what you get for a malformed zip file. There's no
> need to return anything else.

[adg, 2012-10-30 15:38:07]

LGTM

[dfc, 2012-10-30 16:51:21]

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

http://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/reader.go...
src/pkg/archive/zip/reader.go:23: ErrCorruptExtra = errors.New("zip: corrupt
extra data")
On 2012/10/29 21:58:52, bradfitz wrote:
> is this common enough to warrant its own error type variable?  the first two
> seem like yes. the checksum one, maybe. this one is starting to really stretch
> it.  I'd prefer to make this lowercase for now, and export it later if anybody
> actually cares.

Done.

http://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/zip_test.go
File src/pkg/archive/zip/zip_test.go (right):

http://codereview.appspot.com/6811048/diff/6002/src/pkg/archive/zip/zip_test....
src/pkg/archive/zip/zip_test.go:183: defer z.Close()
On 2012/10/29 21:58:52, bradfitz wrote:
> don't you have to do this after fh.Write and before buf.Bytes to ensure a
flush
> happens?  and check the return value from it.

Done.

[dfc, 2012-10-30 16:52:04]

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

archive/zip: handle corrupt extra data records

Fixes issue 4302.

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