FAQ

Search Discussions

  • Dave at Oct 29, 2012 at 9:54 pm
    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello golang-dev@googlegroups.com,

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


    Description:
    archive/zip: handle corrupt extra data records

    Fixes issue 4302.

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

    Affected files:
    M src/pkg/archive/zip/reader.go
    M src/pkg/archive/zip/zip_test.go


    Index: src/pkg/archive/zip/reader.go
    ===================================================================
    --- a/src/pkg/archive/zip/reader.go
    +++ b/src/pkg/archive/zip/reader.go
    @@ -17,9 +17,10 @@
    )

    var (
    - ErrFormat = errors.New("zip: not a valid zip file")
    - ErrAlgorithm = errors.New("zip: unsupported compression algorithm")
    - ErrChecksum = errors.New("zip: checksum error")
    + ErrFormat = errors.New("zip: not a valid zip file")
    + ErrAlgorithm = errors.New("zip: unsupported compression algorithm")
    + ErrChecksum = errors.New("zip: checksum error")
    + ErrCorruptExtra = errors.New("zip: corrupt extra data")
    )

    type Reader struct {
    @@ -241,6 +242,9 @@
    for len(b) > 0 {
    tag := b.uint16()
    size := b.uint16()
    + if int(size) > len(b) {
    + return ErrCorruptExtra
    + }
    if tag == zip64ExtraId {
    // update directory values from the zip64 extra block
    eb := readBuf(b)
    Index: src/pkg/archive/zip/zip_test.go
    ===================================================================
    --- a/src/pkg/archive/zip/zip_test.go
    +++ b/src/pkg/archive/zip/zip_test.go
    @@ -173,3 +173,39 @@
    t.Errorf("UncompressedSize64 %d, want %d", got, want)
    }
    }
    +
    +const timeFormat = "20060102T150405.000.txt"
    +
    +// Issue 4302.
    +func TestInvalidExtraHedaer(t *testing.T) {
    + var buf bytes.Buffer
    + func() {
    + z := NewWriter(&buf)
    + defer z.Close()
    +
    + ts := time.Now()
    + filename := ts.Format(timeFormat)
    +
    + h := FileHeader{
    + Name: filename,
    + Method: Deflate,
    + Extra: []byte(ts.Format(time.RFC3339Nano)), // missing tag and len
    + }
    + h.SetModTime(ts)
    +
    + fh, err := z.CreateHeader(&h)
    + if err != nil {
    + t.Fatalf("Error creating header: %v", err)
    + }
    + _, err = fh.Write([]byte("hi"))
    + if err != nil {
    + t.Fatalf("Error writing content: %v", err)
    + }
    + }()
    + b := buf.Bytes()
    + // TODO(dfc) revision when bytes.Buffer implements ReadAt
    + _, err := NewReader(bytes.NewReader(b), int64(len(b)))
    + if err == nil {
    + t.Fatal("expected ErrCorruptExtra")
    + }
    +}
  • Dave at Oct 29, 2012 at 9:54 pm
    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.go#newcode182
    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.go#newcode204
    src/pkg/archive/zip/zip_test.go:204: }()
    On 2012/10/29 21:52:44, bradfitz wrote:
    why?
    Done.

    https://codereview.appspot.com/6811048/
  • Dave at Oct 29, 2012 at 9:54 pm
    Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6811048/
  • Bradfitz at Oct 29, 2012 at 9:58 pm
    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.go#newcode23
    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.go#newcode183
    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.

    https://codereview.appspot.com/6811048/
  • Dave at Oct 30, 2012 at 1:26 pm
    Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6811048/
  • Bradfitz at Oct 30, 2012 at 1:42 pm
    LGTM



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

    https://codereview.appspot.com/6811048/diff/12002/src/pkg/archive/zip/zip_test.go#newcode206
    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.

    https://codereview.appspot.com/6811048/
  • Dave Cheney at Oct 30, 2012 at 2:12 pm

    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.
  • Brad Fitzpatrick at Oct 30, 2012 at 1:52 pm

    On Tue, Oct 30, 2012 at 2:46 PM, Dave Cheney 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 at Oct 30, 2012 at 2:04 pm
    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.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.

    https://codereview.appspot.com/6811048/
  • Brad Fitzpatrick at Oct 30, 2012 at 2:03 pm

    On Tue, Oct 30, 2012 at 2:56 PM, 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.
  • Andrew Gerrand at Oct 30, 2012 at 2:27 pm

    On 31 October 2012 00:57, Brad Fitzpatrick wrote:
    On Tue, Oct 30, 2012 at 2:56 PM, 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.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.
    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.
  • Dave Cheney at Oct 30, 2012 at 2:28 pm
    Thank you for your comments. PTAL.
    On Wed, Oct 31, 2012 at 1:00 AM, Andrew Gerrand wrote:
    On 31 October 2012 00:57, Brad Fitzpatrick wrote:
    On Tue, Oct 30, 2012 at 2:56 PM, 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.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.
    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 at Oct 30, 2012 at 3:45 pm
  • Dave at Oct 30, 2012 at 4:57 pm
    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#newcode23
    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.go#newcode183
    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.

    http://codereview.appspot.com/6811048/
  • Dave at Oct 30, 2012 at 4:58 pm
    *** 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


    http://codereview.appspot.com/6811048/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 29, '12 at 9:52p
activeOct 30, '12 at 4:58p
posts16
users3
websitegolang.org

3 users in discussion

Dave: 8 posts Brad Fitzpatrick: 5 posts Adg: 3 posts

People

Translate

site design / logo © 2021 Grokbase