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.

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

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

Comments

@c12h
Copy link

@c12h 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 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 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

@gopherbot gopherbot commented Feb 23, 2018

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

@dsnet
Copy link
Member

@dsnet dsnet commented Feb 23, 2018

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 and removed NeedsInvestigation labels Feb 23, 2018
@gopherbot gopherbot closed this in 9697a11 Feb 23, 2018
@dsnet dsnet reopened this Feb 23, 2018
@dsnet dsnet removed their assignment Feb 27, 2018
@andybons
Copy link
Member

@andybons andybons commented Mar 27, 2018

CL 96895 OK for Go 1.10.1

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 27, 2018

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
…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>
@andybons andybons closed this Mar 29, 2018
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.