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/tar: rounding timestamps to the nearest second may result in archives from the future #48275

Closed
milahu opened this issue Sep 9, 2021 · 3 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.

Comments

@milahu
Copy link

milahu commented Sep 9, 2021

when a tar archive, generated by archive/tar/writer.go, is extracted immediately,
tar shows warnings that some timestamps are in the future

downstream issue: ipfs/kubo#8406
downstream workaround: ipfs/go-ipfs-files#41

probably here, Floor should be used

// Round ModTime and ignore AccessTime and ChangeTime unless
// the format is explicitly chosen.
// This ensures nominal usage of WriteHeader (without specifying the format)
// does not always result in the PAX format being chosen, which
// causes a 1KiB increase to every header.
if tw.hdr.Format == FormatUnknown {
tw.hdr.ModTime = tw.hdr.ModTime.Round(time.Second)
tw.hdr.AccessTime = time.Time{}
tw.hdr.ChangeTime = time.Time{}
}

@dsnet
Copy link
Member

dsnet commented Sep 9, 2021

I don't think there's a right answer here. The current logic is entirely reasonable as it optimizes for accuracy at the cost of occasionally being in the future for 500ms. The suggested logic is 500ms less accurate, but avoids being in the future. Both are reasonable and I don't think it's a compelling reason to change, since this will probably break many use-cases that have come to depend on archive/tar for deterministic outputs (see Hyrum's Law).

Have you considered using FormatPAX? That supports high-precision timestamps and may entirely avoid this problem. Also, whatever is flagging archives from the future should probably tolerate an error on the same order as the smallest representable time unit (1s in this case).

@dsnet dsnet changed the title archive/tar/writer.go: tar: time stamp is in the future archive/tar: rounding timestamps to the nearest second may result in archives from the future Sep 9, 2021
@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 9, 2021
@milahu
Copy link
Author

milahu commented Sep 9, 2021

this will probably break many use-cases that have come to depend on archive/tar for deterministic outputs (see Hyrum's Law).

yepp, api consumers hate surprises

its just unexpected behavior, cos usually float-to-int casts mean rounding down

Have you considered using FormatPAX? That supports high-precision timestamps and may entirely avoid this problem.

workaround in ipfs: pass integer time to archive/tar

 func writeFileHeader(w *tar.Writer, fpath string, size uint64) error {
 	return w.WriteHeader(&tar.Header{
 		Name:     fpath,
 		Size:     int64(size),
 		Typeflag: tar.TypeReg,
 		Mode:     0644,
-		ModTime:  time.Now(),
+		ModTime:  time.Now().Truncate(time.Second),
 	})
 }

whatever is flagging archives from the future should probably tolerate an error on the same order as the smallest representable time unit (1s in this case).

yes. in this case nixos unpackFile → _defaultUnpack → tar xf "$fn"
this should be tar xf "$fn" --warning=no-timestamp

thanks! : )

closing

@milahu milahu closed this as completed Sep 9, 2021
@dsnet
Copy link
Member

dsnet commented Sep 9, 2021

That seems like an entirely reasonable and easy workaround.

@golang golang locked and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants