[go] archive/zip: update extended timestamp field if exists
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
archive/zip: update extended timestamp field if exists
Fixes #61572
Change-Id: If008014ea970e8cc92a8fe4b347f9c3e9446a258
GitHub-Last-Rev: 877c98247e09910ea931e5625255f744559aa90a
GitHub-Pull-Request: golang/go#61622
---
M src/archive/zip/writer.go
M src/archive/zip/zip_test.go
2 files changed, 99 insertions(+), 1 deletion(-)
diff --git a/src/archive/zip/writer.go b/src/archive/zip/writer.go
index 3b23cc3..f60a236 100644
--- a/src/archive/zip/writer.go
+++ b/src/archive/zip/writer.go
@@ -326,7 +326,12 @@
eb.uint16(5) // Size: SizeOf(uint8) + SizeOf(uint32)
eb.uint8(1) // Flags: ModTime
eb.uint32(mt) // ModTime
- fh.Extra = append(fh.Extra, mbuf[:]...)
+
+ if offset := extraFieldOffset(fh.Extra, extTimeExtraID); offset != -1 {
+ copy(fh.Extra[offset:], mbuf[:])
+ } else {
+ fh.Extra = append(fh.Extra, mbuf[:]...)
+ }
}
var (
@@ -383,6 +388,22 @@
return ow, nil
}
+func extraFieldOffset(extra []byte, tag uint16) int {
+ offset := 0
+ for buf := readBuf(extra); len(buf) >= 4; { // need at least tag and size
+ if fieldTag := buf.uint16(); fieldTag == tag {
+ return offset
+ }
+ fieldSize := int(buf.uint16())
+ if len(buf) < fieldSize {
+ break
+ }
+ buf.sub(fieldSize)
+ offset += 4 + fieldSize
+ }
+ return -1
+}
+
func writeHeader(w io.Writer, h *header) error {
const maxUint16 = 1<<16 - 1
if len(h.Name) > maxUint16 {
diff --git a/src/archive/zip/zip_test.go b/src/archive/zip/zip_test.go
index 7d1de07..e9d06ac 100644
--- a/src/archive/zip/zip_test.go
+++ b/src/archive/zip/zip_test.go
@@ -826,3 +826,80 @@
}
return len(p), nil
}
+
+// Issue 61572
+func TestWriterModifiedTime(t *testing.T) {
+ modified := time.Date(2023, 07, 27, 00, 41, 57, 0, timeZone(+1*time.Hour))
+ extendedTimestampField := []byte{
+ // tag 0x5455 size 5, extended timestamp, encodes modified
+ 0x55, 0x54, 5, 0, 0x01, 0x45, 0xaf, 0xc1, 0x64,
+ }
+
+ for _, tc := range []struct {
+ name string
+ extra, want []byte
+ }{
+ {
+ name: "adds modified time to empty extra",
+ extra: nil,
+ want: extendedTimestampField,
+ },
+ {
+ name: "adds modified time if extended timestamp does not exist",
+ extra: []byte{
+ // tag 0x9999 size 5
+ 0x99, 0x99, 5, 0, 0, 1, 2, 3, 4,
+ // tag 0xaaaa size 0
+ 0xaa, 0xaa, 0, 0,
+ },
+ want: append([]byte{
+ 0x99, 0x99, 5, 0, 0, 1, 2, 3, 4,
+ 0xaa, 0xaa, 0, 0},
+ extendedTimestampField...),
+ },
+ {
+ name: "updates modified time if extended timestamp exists",
+ extra: []byte{
+ // tag 0x9999 size 5
+ 0x99, 0x99, 5, 0, 0, 1, 2, 3, 4,
+ // tag 0x5455 size 5, extended timestamp, encodes time.Date(2017, 10, 31, 21, 11, 57, 0, timeZone(-7*time.Hour))
+ 0x55, 0x54, 5, 0, 0x01, 0x8d, 0x49, 0xf9, 0x59,
+ // tag 0xaaaa size 0
+ 0xaa, 0xaa, 0, 0,
+ },
+ want: append(append([]byte{
+ 0x99, 0x99, 5, 0, 0, 1, 2, 3, 4},
+ extendedTimestampField...),
+ 0xaa, 0xaa, 0, 0),
+ },
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ h := &FileHeader{
+ Name: "issue61572.txt",
+ Modified: modified,
+ Extra: tc.extra,
+ }
+
+ var buf bytes.Buffer
+ w := NewWriter(&buf)
+ if _, err := w.CreateHeader(h); err != nil {
+ t.Fatalf("unexpected CreateHeader error: %v", err)
+ }
+ if err := w.Close(); err != nil {
+ t.Fatalf("unexpected Close error: %v", err)
+ }
+
+ in := buf.Bytes()
+ zf, err := NewReader(bytes.NewReader(in), int64(len(in)))
+ if err != nil {
+ t.Fatalf("unexpected NewReader error: %v", err)
+ }
+
+ if got := zf.File[0].FileHeader.Extra; !bytes.Equal(tc.want, got) {
+ t.Logf("want: % x", tc.want)
+ t.Logf(" got: % x", got)
+ t.Error("wrong header extra")
+ }
+ })
+ }
+}
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Gerrit Bot uploaded patch set #2 to this change.
archive/zip: update extended timestamp field if exists
Fixes #61572
Change-Id: If008014ea970e8cc92a8fe4b347f9c3e9446a258
GitHub-Last-Rev: 80fe15d789af252ef1125fbf6c3f8746f9a7b63a
GitHub-Pull-Request: golang/go#61622
---
M src/archive/zip/writer.go
M src/archive/zip/zip_test.go
2 files changed, 100 insertions(+), 1 deletion(-)
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Yastrebov (Gerrit)
1 comment:
Commit Message:
Patch Set #1, Line 7:
archive/zip: update extended timestamp field if existsAlternative CL 513437 ignores modified time
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Joseph Tsai.
Gerrit Bot uploaded patch set #3 to this change.
archive/zip: update extended timestamp field if exists
Fixes #61572
Change-Id: If008014ea970e8cc92a8fe4b347f9c3e9446a258
GitHub-Last-Rev: f2f7a157f44549108dae704314df935f9d1ba311
GitHub-Pull-Request: golang/go#61622
---
M src/archive/zip/writer.go
M src/archive/zip/zip_test.go
2 files changed, 188 insertions(+), 3 deletions(-)
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Yastrebov (Gerrit)
Attention is currently required from: Joseph Tsai.
Patch Set #1, Line 7:
archive/zip: update extended timestamp field if existsAlternative CL 513437 ignores modified time
Done
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Joseph Tsai.
Gerrit Bot uploaded patch set #4 to this change.
archive/zip: update extended timestamp field if exists
Fixes #61572
Change-Id: If008014ea970e8cc92a8fe4b347f9c3e9446a258
GitHub-Last-Rev: a399afb2ed36dccacc5d1f785127a0659e95ccb6
GitHub-Pull-Request: golang/go#61622
---
M src/archive/zip/writer.go
M src/archive/zip/zip_test.go
2 files changed, 171 insertions(+), 3 deletions(-)
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
1 comment:
File src/archive/zip/writer.go:
Patch Set #4, Line 404:
if fieldSize != 5 {What happens if AcTime or CrTime is present? This is valid, but means the size may not be 5.
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Yastrebov (Gerrit)
Attention is currently required from: Joseph Tsai.
Patch Set #4, Line 404:
if fieldSize != 5 {What happens if AcTime or CrTime is present? This is valid, but means the size may not be 5.
Missed that part, sorry.
Per https://mdfs.net/Docs/Comp/Archiving/Zip/ExtraField
> Those times that are present will appear in the order indicated, but
> any combination of times may be omitted. (Creation time may be
> present without access time, for example.) TSize should equal
> (1 + 4*(number of set bits in Flags)), as the block is currently
> defined. Other timestamps may be added in the future.Since the caller only writes ModTime - should we overwrite the whole field regardless of its size (i.e. potentially loosing previous AcTime or CrTime values)?
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Alexander Yastrebov.
1 comment:
File src/archive/zip/writer.go:
Patch Set #4, Line 404:
if fieldSize != 5 {
Missed that part, sorry. […]
I'd argue the most correct thing to do is to just upsert the ModTime field.
If the other fields are present, we should maintain them if possible.At a high-level:
```
if the "extended timestamp" field is missing
then add the entire "extended timestamp" field as we previously did
else if ModTime field is present
then update the ModTime field alone
else if ModTime field is missing
then update the flags to include and inject the ModTime field
```
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Yastrebov (Gerrit)
Attention is currently required from: Joseph Tsai.
1 comment:
File src/archive/zip/writer.go:
Patch Set #4, Line 404:
if fieldSize != 5 {
I'd argue the most correct thing to do is to just upsert the ModTime field. […]
It also says `The central-header extra field contains the modification time only, or no timestamp at all.` but I do not see a code to strip down AcTime/CrTime during central directory write. I guess the file that has them would not roundtrip read-write.
To view, visit change 513836. To unsubscribe, or for help writing mail filters, visit settings.