Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

archive/zip: takes mtime from wrong part of infoZipUnixExtraID extra headers #23901

Closed
c12h opened this issue Feb 18, 2018 · 5 comments
Closed

archive/zip: takes mtime from wrong part of infoZipUnixExtraID extra headers #23901

c12h opened this issue Feb 18, 2018 · 5 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.10.1

Comments

@c12h
Copy link

c12h commented Feb 18, 2018

Looking at src/archive/zip/reader.go from the Go 1.10 kit, I see that readDirectory() gets the modified time from the first 32 bits after an infoZipUnixExtraID's length field:

    case infoZipUnixExtraID:
        if len(fieldBuf) < 4 {
            continue parseExtras
        }
        ts := int64(fieldBuf.uint32()) // ModTime since Unix epoch
        modified = time.Unix(ts, 0)

But according to file unzip60/proginfo/extrafld.txt from
https://sourceforge.net/projects/infozip/files/UnZip%206.x%20%28latest%29/UnZip%206.0/unzip60.tar.gz/download, the layout of an infoZipUnixExtraID extra is:

    0x5855  Short   tag for this extra block type ("UX")
    TSize   Short   total data size for this block
    AcTime  Long    time of last access (GMT/UTC)
    ModTime Long    time of last modification (GMT/UTC)

so the current code gets the last access time, not the last modification time.

In fact, the ModTime field in an infoZipUnixExtraID (0x5855) extra header has the same offset and size at
the ModTime field in an unixExtraID (0x000d) block, so both cases can use the same code:
zip-bug.patch.gz

This is (probably) not an important bug, because the infoZipUnixExtraID (0x5855) block has long been deprecated in favor of extTimeExtraID (0x5455) and the Unix type 2 extra block (0x7855).
@dsnet dsnet self-assigned this Feb 18, 2018
@dsnet dsnet added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 18, 2018
@dsnet dsnet added this to the Go1.11 milestone Feb 18, 2018
@ianlancetaylor ianlancetaylor changed the title archive/zip takes mtime from wrong part of infoZipUnixExtraID extra headers archive/zip: takes mtime from wrong part of infoZipUnixExtraID extra headers Feb 18, 2018
@dsnet
Copy link
Member

dsnet commented Feb 23, 2018

This is (probably) not an important bug, because the infoZipUnixExtraID (0x5855) block has long been deprecated in favor of extTimeExtraID (0x5455) and the Unix type 2 extra block (0x7855).

You say that, but the default ZIP archiver on OSX (at least on Sierra) continues to generate that field...

@dsnet dsnet modified the milestones: Go1.11, Go1.10.1 Feb 23, 2018
@gopherbot
Copy link

Change https://golang.org/cl/96895 mentions this issue: archive/zip: fix handling of Info-ZIP Unix extended timestamps

@dsnet
Copy link
Member

dsnet commented Feb 23, 2018 •
edited

Fix is out. Moving to 1.10.1 milestone for consideration in the point release. The new time-parsing logic was added in Go1.10, and it would be unfortunate to continue reporting the wrong timestamp.

@dsnet dsnet added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 23, 2018
@dsnet dsnet reopened this Feb 23, 2018
@dsnet dsnet removed their assignment Feb 27, 2018
@andybons andybons mentioned this issue Mar 27, 2018
Closed
@andybons
Copy link
Member

CL 96895 OK for Go 1.10.1

@andybons andybons added CherryPickApproved Used during the release process for point releases and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 27, 2018
@gopherbot
Copy link

Change https://golang.org/cl/102782 mentions this issue: [release-branch.go1.10] archive/zip: fix handling of Info-ZIP Unix extended timestamps

gopherbot pushed a commit that referenced this issue Mar 29, 2018
@dsnet @andybons
[release-branch.go1.10] archive/zip: fix handling of Info-ZIP Unix ex…
176900a 
…tended timestamps

The Info-ZIP Unix1 extra field is specified as such:
>>>
Value    Size   Description
-----    ----   -----------
0x5855   Short  tag for this extra block type ("UX")
TSize    Short  total data size for this block
AcTime   Long   time of last access (GMT/UTC)
ModTime  Long   time of last modification (GMT/UTC)
<<<

The previous handling was incorrect in that it read the AcTime field
instead of the ModTime field.

The test-osx.zip test unfortunately locked in the wrong behavior.
Manually parsing that ZIP file shows that the encoded MS-DOS
date and time are 0x4b5f and 0xa97d, which corresponds with a
date of 2017-10-31 21:11:58, which matches the correct mod time
(off by 1 second due to MS-DOS timestamp resolution).

Fixes #23901

Change-Id: I567824c66e8316b9acd103dbecde366874a4b7ef
Reviewed-on: https://go-review.googlesource.com/96895
Run-TryBot: Joe Tsai <joetsai@google.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/102782
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Mar 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.10.1
Development

No branches or pull requests
4 participants
@andybons @dsnet @gopherbot @c12h