code review 117550044: archive/zip: accept bogus trailing zeros in extras

archive/zip: accept bogus trailing zeros in extras

Popular tools both add incorrect trailing zeroes to the zip
extras, and popular tools accept trailing zeros. We seemed to
be the only ones being strict here. Stop being strict. :(

Fixes Issue 8186

[bradfitz, 2014-08-04 18:44:23]

Hello adg@golang.org (cc: frohrweck@google.com,
golang-codereviews@googlegroups.com),

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

[ruiu, 2014-08-04 18:51:24]

https://codereview.appspot.com/117550044/diff/40001/src/pkg/archive/zip/reade...
File src/pkg/archive/zip/reader.go (right):

https://codereview.appspot.com/117550044/diff/40001/src/pkg/archive/zip/reade...
src/pkg/archive/zip/reader.go:273: if len(b) != 0 {
I think you can remove this if.

[bradfitz, 2014-08-04 20:33:02]

Done.



On Mon, Aug 4, 2014 at 11:51 AM, <ruiu@google.com> wrote:

>
> https://codereview.appspot.com/117550044/diff/40001/src/
> pkg/archive/zip/reader.go
> File src/pkg/archive/zip/reader.go (right):
>
> https://codereview.appspot.com/117550044/diff/40001/src/
> pkg/archive/zip/reader.go#newcode273
> src/pkg/archive/zip/reader.go:273: if len(b) != 0 {
> I think you can remove this if.
>
> https://codereview.appspot.com/117550044/
>

[bradfitz, 2014-08-04 20:33:03]

Hello adg@golang.org, ruiu@google.com (cc: frohrweck@google.com,
golang-codereviews@googlegroups.com),

Please take another look.

[ruiu, 2014-08-04 20:37:38]

LGTM

https://codereview.appspot.com/117550044/diff/60001/src/pkg/archive/zip/reade...
File src/pkg/archive/zip/reader_test.go (right):

https://codereview.appspot.com/117550044/diff/60001/src/pkg/archive/zip/reade...
src/pkg/archive/zip/reader_test.go:533: }
I'd probably add a test to verify that it returns an ErrFormat if the padding is
not zero, but it may be too much. I'll leave it to you.

[adg, 2014-08-04 22:16:22]

LGTM

Sigh.

On Tuesday, 5 August 2014, <ruiu@google.com> wrote:

> LGTM
>
>
> https://codereview.appspot.com/117550044/diff/60001/src/
> pkg/archive/zip/reader_test.go
> File src/pkg/archive/zip/reader_test.go (right):
>
> https://codereview.appspot.com/117550044/diff/60001/src/
> pkg/archive/zip/reader_test.go#newcode533
> src/pkg/archive/zip/reader_test.go:533: }
> I'd probably add a test to verify that it returns an ErrFormat if the
> padding is not zero, but it may be too much. I'll leave it to you.
>
> https://codereview.appspot.com/117550044/
>

[dfc, 2014-08-04 22:21:42]

LGTM. I agree, why have a standard if the actual implementation is
determined by mob rule?

I was concerned that this would consume an arbitrary number of zeros, but
the code before asserts that this is bounded by the size of f.Extra, so I'm
guessing this padding is just rounding up to the size of a word boundary.
On 5 Aug 2014 08:16, "Andrew Gerrand" <adg@golang.org> wrote:

> LGTM
>
> Sigh.
>
> On Tuesday, 5 August 2014, <ruiu@google.com> wrote:
>
>> LGTM
>>
>>
>> https://codereview.appspot.com/117550044/diff/60001/src/
>> pkg/archive/zip/reader_test.go
>> File src/pkg/archive/zip/reader_test.go (right):
>>
>> https://codereview.appspot.com/117550044/diff/60001/src/
>> pkg/archive/zip/reader_test.go#newcode533
>> src/pkg/archive/zip/reader_test.go:533: }
>> I'd probably add a test to verify that it returns an ErrFormat if the
>> padding is not zero, but it may be too much. I'll leave it to you.
>>
>> https://codereview.appspot.com/117550044/
>>
>  --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[bradfitz, 2014-08-04 22:22:22]

Yes, in practice it's 0-3 bytes of extra zeros.



On Mon, Aug 4, 2014 at 3:21 PM, Dave Cheney <dave@cheney.net> wrote:

> LGTM. I agree, why have a standard if the actual implementation is
> determined by mob rule?
>
> I was concerned that this would consume an arbitrary number of zeros, but
> the code before asserts that this is bounded by the size of f.Extra, so I'm
> guessing this padding is just rounding up to the size of a word boundary.
> On 5 Aug 2014 08:16, "Andrew Gerrand" <adg@golang.org> wrote:
>
>> LGTM
>>
>> Sigh.
>>
>> On Tuesday, 5 August 2014, <ruiu@google.com> wrote:
>>
>>> LGTM
>>>
>>>
>>> https://codereview.appspot.com/117550044/diff/60001/src/
>>> pkg/archive/zip/reader_test.go
>>> File src/pkg/archive/zip/reader_test.go (right):
>>>
>>> https://codereview.appspot.com/117550044/diff/60001/src/
>>> pkg/archive/zip/reader_test.go#newcode533
>>> src/pkg/archive/zip/reader_test.go:533: }
>>> I'd probably add a test to verify that it returns an ErrFormat if the
>>> padding is not zero, but it may be too much. I'll leave it to you.
>>>
>>> https://codereview.appspot.com/117550044/
>>>
>>  --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>

[adg, 2014-08-04 22:35:10]

On 5 August 2014 08:21, Dave Cheney <dave@cheney.net> wrote:

> why have a standard if the actual implementation is determined by mob rul


There is no zip standard.

[bradfitz, 2014-08-04 23:12:59]

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

archive/zip: accept bogus trailing zeros in extras

Popular tools both add incorrect trailing zeroes to the zip
extras, and popular tools accept trailing zeros. We seemed to
be the only ones being strict here. Stop being strict. :(

Fixes Issue 8186

LGTM=ruiu, adg, dave
R=adg, ruiu, dave
CC=frohrweck, golang-codereviews
https://codereview.appspot.com/117550044