archive/zip: use base offset 0 if it has a valid entry

In CL 408734 we introduced a fall back to base offset 0 if reading a
directory entry at the computed base offset failed. We have now found
a file in the wild for which the computed base offset is incorrect,
but happens to refer to a valid directory entry. In this CL, we change
the fallback such that if the first directory header relative to base
offset 0 is valid, we just use base offset 0.

Change-Id: Ia9ace20c1065d1f651035f16f7d91d741ab1dbf4
Reviewed-on: https://go-review.googlesource.com/c/go/+/461598
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Joseph Tsai <joetsai@digital-static.net>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>

3 files changed, 32 insertions, 14 deletions

diff --git a/src/archive/zip/reader.go b/src/archive/zip/reader.go
index c29837836b..3e96d0ecc9 100644
--- a/src/archive/zip/reader.go
+++ b/src/archive/zip/reader.go
@@ -151,20 +151,6 @@ func (z *Reader) init(r io.ReaderAt, size int64) error {
 	for {
 		f := &File{zip: z, zipr: r}
 		err = readDirectoryHeader(f, buf)
-
-		// For compatibility with other zip programs,
-		// if we have a non-zero base offset and can't read
-		// the first directory header, try again with a zero
-		// base offset.
-		if err == ErrFormat && z.baseOffset != 0 && len(z.File) == 0 {
-			z.baseOffset = 0
-			if _, err = rs.Seek(int64(end.directoryOffset), io.SeekStart); err != nil {
-				return err
-			}
-			buf.Reset(rs)
-			continue
-		}
-
 		if err == ErrFormat || err == io.ErrUnexpectedEOF {
 			break
 		}
@@ -627,6 +613,20 @@ func readDirectoryEnd(r io.ReaderAt, size int64) (dir *directoryEnd, baseOffset
 	if o := baseOffset + int64(d.directoryOffset); o < 0 || o >= size {
 		return nil, 0, ErrFormat
 	}
+
+	// If the directory end data tells us to use a non-zero baseOffset,
+	// but we would find a valid directory entry if we assume that the
+	// baseOffset is 0, then just use a baseOffset of 0.
+	// We've seen files in which the directory end data gives us
+	// an incorrect baseOffset.
+	if baseOffset > 0 {
+		off := int64(d.directoryOffset)
+		rs := io.NewSectionReader(r, off, size-off)
+		if readDirectoryHeader(&File{}, rs) == nil {
+			baseOffset = 0
+		}
+	}
+
 	return d, baseOffset, nil
 }
 
diff --git a/src/archive/zip/reader_test.go b/src/archive/zip/reader_test.go
index 1594b2648e..70ad260cc5 100644
--- a/src/archive/zip/reader_test.go
+++ b/src/archive/zip/reader_test.go
@@ -127,6 +127,24 @@ var tests = []ZipTest{
 		},
 	},
 	{
+		Name:    "test-badbase.zip",
+		Comment: "This is a zipfile comment.",
+		File: []ZipTestFile{
+			{
+				Name:     "test.txt",
+				Content:  []byte("This is a test text file.\n"),
+				Modified: time.Date(2010, 9, 5, 12, 12, 1, 0, timeZone(+10*time.Hour)),
+				Mode:     0644,
+			},
+			{
+				Name:     "gophercolor16x16.png",
+				File:     "gophercolor16x16.png",
+				Modified: time.Date(2010, 9, 5, 15, 52, 58, 0, timeZone(+10*time.Hour)),
+				Mode:     0644,
+			},
+		},
+	},
+	{
 		Name:   "r.zip",
 		Source: returnRecursiveZip,
 		File: []ZipTestFile{
diff --git a/src/archive/zip/testdata/test-badbase.zip b/src/archive/zip/testdata/test-badbase.zip
new file mode 100644
index 0000000000..245a62cb6d
--- /dev/null
+++ b/src/archive/zip/testdata/test-badbase.zip