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: Writer should only use extended timestamps for non-zero times #17403

Closed
dsnet opened this issue Oct 11, 2016 · 7 comments
Closed

archive/zip: Writer should only use extended timestamps for non-zero times #17403

dsnet opened this issue Oct 11, 2016 · 7 comments
Assignees
Milestone

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Oct 11, 2016

4c79ed5 caused a regression where a round-trip zip file does not preserve the same ModTime. The reason is because the logic for using extra timestamp is always applied in the Writer no matter what.

This is an issue for when the user doesn't set the ModifiedDate and ModifiedTime fields at all. Causing the Writer to convert a zero MS-DOS timestamp (gibberish) to a Unix timestamp (now gibberish) and back again. The fix is to only apply Extra timestamp only if the MS-DOS timestamp is non-zero.

See:

mt := uint32(h.FileHeader.ModTime().Unix())
var mbuf [9]byte // 2x uint16 + uint8 + uint32
eb := writeBuf(mbuf[:])
eb.uint16(exttsExtraId)
eb.uint16(5) // size = uint8 + uint32
eb.uint8(1) // flags = modtime
eb.uint32(mt) // ModTime
h.Extra = append(h.Extra, mbuf[:]...)

/cc @mattn

@dsnet dsnet added the NeedsFix label Oct 11, 2016
@dsnet dsnet added this to the Go1.8 milestone Oct 11, 2016
@dsnet dsnet self-assigned this Oct 11, 2016
@mattn
Copy link
Member

@mattn mattn commented Oct 11, 2016

What do you mean same? Timestamp in the zip file was always preserved without timezone. It was really problem.

@dsnet
Copy link
Member Author

@dsnet dsnet commented Oct 11, 2016

This fails on tip, but passes on Go 1.7

https://play.golang.org/p/-ckATAnaEn

@mattn
Copy link
Member

@mattn mattn commented Oct 11, 2016

Thanks for looking this. Are you trying to fix to avoid preserve ModifiedTime=0 ModifiedDate=0 if the file already having timestamp?

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 11, 2016

CL https://golang.org/cl/30811 mentions this issue.

@gopherbot gopherbot closed this in 3522053 Oct 11, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 20, 2016

CL https://golang.org/cl/34651 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 20, 2016
This change reverts the following CLs:
	CL/18274: handle mtime in NTFS/UNIX/ExtendedTS extra fields
	CL/30811: only use Extended Timestamp on non-zero MS-DOS timestamps

We are reverting support for extended timestamps since the support was not
not complete. CL/18274 added full support for reading extended timestamp fields
and minimal support for writing them. CL/18274 is incomplete because it made
no changes to the FileHeader struct, so timezone information was lost when
reading and/or writing.

While CL/18274 was a step in the right direction, we should provide full
support for high precision timestamps in both the reader and writer.
This will probably require that we add a new field of type time.Time.
The complete fix is too involved to add in the time remaining for Go 1.8
and will be completed in Go 1.9.

Updates #10242
Updates #17403
Updates #18359
Fixes #18378

Change-Id: Icf6d028047f69379f7979a29bfcb319a02f4783e
Reviewed-on: https://go-review.googlesource.com/34651
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 6, 2017

Change https://golang.org/cl/44394 mentions this issue: archive/zip: Store the main timestamp as local time.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 6, 2017

Change https://golang.org/cl/36078 mentions this issue: archive/zip: handle mtime in NTFS/UNIX/ExtendedTS extra fields

@golang golang locked and limited conversation to collaborators Nov 6, 2018
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
3 participants
You can’t perform that action at this time.