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: f.ModTime()'s yyyy/mm/dd hh:mm:ss should match Go 1.9 #22738

Closed
rsc opened this issue Nov 15, 2017 · 3 comments
Closed

archive/zip: f.ModTime()'s yyyy/mm/dd hh:mm:ss should match Go 1.9 #22738

rsc opened this issue Nov 15, 2017 · 3 comments

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Nov 15, 2017

The result of f.ModTime().Format("2006-01-02 15:04:05") has changed from Go 1.9 to Go 1.10. Given that ModTime is working with bit fields that encode yyyy/mm/dd hh:mm:ss, printing those fields should continue to reproduce their actual values (as Go 1.9 and earlier versions did).

https://swtch.com/tmp/ziptime.zip is this file:

00000000  50 4b 03 04 14 00 08 00  00 00 00 98 9f 27 00 00  |PK...........'..|
00000010  00 00 00 00 00 00 00 00  00 00 04 00 09 00 66 69  |..............fi|
00000020  6c 65 55 54 05 00 01 80  43 6d 38 50 4b 07 08 00  |leUT....Cm8PK...|
00000030  00 00 00 00 00 00 00 00  00 00 00 50 4b 01 02 14  |...........PK...|
00000040  00 14 00 08 00 00 00 00  98 9f 27 00 00 00 00 00  |..........'.....|
00000050  00 00 00 00 00 00 00 04  00 09 00 00 00 00 00 00  |................|
00000060  00 00 00 00 00 00 00 00  00 66 69 6c 65 55 54 05  |.........fileUT.|
00000070  00 01 80 43 6d 38 50 4b  05 06 00 00 00 00 01 00  |...Cm8PK........|
00000080  01 00 3b 00 00 00 3b 00  00 00 00 00              |..;...;.....|
0000008c

The extra field beginning at offset 006d encodes the Unix time 0x386d4380 aka 2000-01-01 00:00:00 UTC. The MS-DOS time fields at offset 000c encode time=0x9800=19:00:00 date=0x279f=1999-12-31, aka what that Unix time displayed as in US Eastern time.

Here's a program that prints the zip.FileInfo's ModTime result:

package main

import (
	"archive/zip"
	"fmt"
	"os"
)

func main() {
	f, _ := os.Open("ziptime.zip")
	info, _ := f.Stat()
	r, _ := zip.NewReader(f, info.Size())
	fmt.Println(r.File[0].ModTime())
	//fmt.Println(r.File[0].Modified)
}

The output has changed since Go 1.9:

$ go1.9 run r.go
1999-12-31 19:00:00 +0000 UTC
$ go run r.go
2000-01-01 00:00:00 +0000 UTC
$ 

This is clearly an improvement: the time instant is correct now and was incorrect in Go 1.9. However, it's also a regression, in that previously if you did t := f.ModTime() and then queried t.Year, t.Month, t.Day, t.Hour, t.Minute, t.Second, they all matched the encoded MS-DOS bits in the zip file. Now they don't.

Obviously the result of f.ModTime must change given the cleanup here. But in the Go 1.9 result the time zone was incorrect and the Year/Month/Day/Hour/Minute/Second was correct. I don't see why Go 1.10 is preserving the incorrect timezone and changing (breaking) the Year/Month/Day/Hour/Minute/Second. It seems like it should be preserving the correct Year/Month/Day/Hour/Minute/Second and changing (fixing) the time zone.

If you uncomment the Go 1.10-specific line in my program above, it prints:

$ go run r.go
2000-01-01 00:00:00 +0000 UTC
1999-12-31 19:00:00 -0500 -0500
$ 

It seems to me that ModTime() should simply return f.Modified directly, without the UTC coercion. That will effect the fix I suggested.

Then the original Go 1.9 vs Go 1.10 comparison becomes:

$ go1.9 run r.go
1999-12-31 19:00:00 +0000 UTC
$ go run r.go
1999-12-31 19:00:00 -0500 -0500
$ 

and that looks like a bug fix (instead of a bug being introduced).

An alternative would be to have ModTime continue to return the different time instant 1999-12-31 19:00:00 +0000 UTC. It's possible that that's an even better idea. One way or another though, the current output should change.

/cc @dsnet

@rsc rsc added this to the Go1.10 milestone Nov 15, 2017
@rsc rsc added the release-blocker label Nov 15, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 15, 2017

Change https://golang.org/cl/78031 mentions this issue: archive/zip: preserve old FileHeader.ModTime behavior

@gopherbot gopherbot closed this in d85a353 Nov 29, 2017
@rsc rsc reopened this Nov 29, 2017
@rsc
Copy link
Contributor Author

@rsc rsc commented Nov 29, 2017

Reopening to track CL with test.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 29, 2017

Change https://golang.org/cl/80635 mentions this issue: archive/zip: add test for Modified vs ModTime behavior

@gopherbot gopherbot closed this in 9cc9f10 Dec 1, 2017
@golang golang locked and limited conversation to collaborators Dec 1, 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
2 participants
You can’t perform that action at this time.