Improve overlapping detection in ZFile.

ZFile now reports when files overlap in a zip. It previously
failed with a VerifyException and now, at least in cases where
the local header can still be read correctly (for example, a
file contained inside another file), it reports a nice error
that allows users to understand what the problem is.

Bug: 69835362
Change-Id: Ib34df31f97805f1d5ec9ba917dce7374ea6a7d22
Test: included

6 files changed, 119 insertions, 1 deletions

diff --git a/src/main/java/com/android/apkzlib/zip/FileUseMap.java b/src/main/java/com/android/apkzlib/zip/FileUseMap.java
index a72a956..8a76878 100644
--- a/src/main/java/com/android/apkzlib/zip/FileUseMap.java
+++ b/src/main/java/com/android/apkzlib/zip/FileUseMap.java
@@ -538,6 +538,43 @@ class FileUseMap {
         return map.lower(entry);
     }
 
+    /**
+     * Obtains the entry that is located after the one provided.
+     *
+     * @param entry the map entry to get the next one for; must belong to the map
+     * @return the entry after the provided one, {@code null} if {@code entry} is the last entry in
+     *     the map
+     */
+    @Nullable
+    FileUseMapEntry<?> after(@Nonnull FileUseMapEntry<?> entry) {
+        Preconditions.checkNotNull(entry, "entry == null");
+
+        return map.higher(entry);
+    }
+
+    /**
+     * Obtains the entry at the given offset.
+     *
+     * @param offset the offset to look for
+     * @return the entry found or {@code null} if there is no entry (not even a free one) at the
+     *     given offset
+     */
+    @Nullable
+    FileUseMapEntry<?> at(long offset) {
+        Preconditions.checkArgument(offset >= 0, "offset < 0");
+        Preconditions.checkArgument(offset < size, "offset >= size");
+
+        FileUseMapEntry<?> entry = map.floor(FileUseMapEntry.makeFree(offset, offset + 1));
+        if (entry == null) {
+            return null;
+        }
+
+        Verify.verify(entry.getStart() <= offset);
+        Verify.verify(entry.getEnd() > offset);
+
+        return entry;
+    }
+
     @Override
     public String toString() {
         StringJoiner j = new StringJoiner(", ");
diff --git a/src/main/java/com/android/apkzlib/zip/ZFile.java b/src/main/java/com/android/apkzlib/zip/ZFile.java
index 1bdb410..9034f4c 100644
--- a/src/main/java/com/android/apkzlib/zip/ZFile.java
+++ b/src/main/java/com/android/apkzlib/zip/ZFile.java
@@ -597,7 +597,7 @@ public class ZFile implements Closeable {
         readCentralDirectory();
 
         /*
-         * Compute where the last file ends. We will need this to compute thee extra offset.
+         * Go over all files and create the usage map, verifying there is no overlap in the files.
          */
         long entryEndOffset;
         long directoryStartOffset;
@@ -625,6 +625,51 @@ public class ZFile implements Closeable {
                  * file.
                  */
 
+                Verify.verify(start >= 0, "start < 0");
+                Verify.verify(end < map.size(), "end >= map.size()");
+
+                FileUseMapEntry<?> found = map.at(start);
+                Verify.verifyNotNull(found);
+
+                // We've got a problem if the found entry is not free or is a free entry but
+                // doesn't cover the whole file.
+                if (!found.isFree() || found.getEnd() < end) {
+                    if (found.isFree()) {
+                        found = map.after(found);
+                        Verify.verify(found != null && !found.isFree());
+                    }
+
+                    Object foundEntry = found.getStore();
+                    Verify.verify(foundEntry != null);
+
+                    // Obtains a custom description of an entry.
+                    IOExceptionFunction<StoredEntry, String> describe =
+                            e ->
+                                    String.format(
+                                            "'%s' (offset: %d, size: %d)",
+                                            e.getCentralDirectoryHeader().getName(),
+                                            e.getCentralDirectoryHeader().getOffset(),
+                                            e.getInFileSize());
+
+                    String overlappingEntryDescription;
+                    if (foundEntry instanceof StoredEntry) {
+                        StoredEntry foundStored = (StoredEntry) foundEntry;
+                        overlappingEntryDescription = describe.apply((StoredEntry) foundEntry);
+                    } else {
+                        overlappingEntryDescription =
+                                "Central Directory / EOCD: "
+                                        + found.getStart()
+                                        + " - "
+                                        + found.getEnd();
+                    }
+
+                    throw new IOException(
+                            "Cannot read entry "
+                                    + describe.apply(entry)
+                                    + " because it overlaps with "
+                                    + overlappingEntryDescription);
+                }
+
                 FileUseMapEntry<StoredEntry> mapEntry = map.add(start, end, entry);
                 entries.put(entry.getCentralDirectoryHeader().getName(), mapEntry);
 
diff --git a/src/test/java/com/android/apkzlib/zip/ZFileTest.java b/src/test/java/com/android/apkzlib/zip/ZFileTest.java
index bce0286..b7f2979 100644
--- a/src/test/java/com/android/apkzlib/zip/ZFileTest.java
+++ b/src/test/java/com/android/apkzlib/zip/ZFileTest.java
@@ -33,6 +33,7 @@ import com.android.apkzlib.zip.utils.CloseableByteSource;
 import com.android.apkzlib.zip.utils.RandomAccessFileUtils;
 import com.google.common.base.Charsets;
 import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
 import com.google.common.hash.Hashing;
 import com.google.common.io.ByteStreams;
 import com.google.common.io.Closer;
@@ -1782,4 +1783,39 @@ public class ZFileTest {
             assertArrayEquals(comment, zf.getEocdComment());
         }
     }
+
+    @Test
+    public void overlappingZipEntries() throws Exception {
+        File myZip = ZipTestUtils.cloneRsrc("overlapping.zip", mTemporaryFolder);
+        try (ZFile zf = new ZFile(myZip)) {
+            fail();
+        } catch (IOException e) {
+            assertTrue(Throwables.getStackTraceAsString(e).contains("overlapping/bbb"));
+            assertTrue(Throwables.getStackTraceAsString(e).contains("overlapping/ddd"));
+            assertFalse(Throwables.getStackTraceAsString(e).contains("Central Directory"));
+        }
+    }
+
+    @Test
+    public void overlappingZipEntryWithCentralDirectory() throws Exception {
+        File myZip = ZipTestUtils.cloneRsrc("overlapping2.zip", mTemporaryFolder);
+        try (ZFile zf = new ZFile(myZip)) {
+            fail();
+        } catch (IOException e) {
+            assertFalse(Throwables.getStackTraceAsString(e).contains("overlapping/bbb"));
+            assertTrue(Throwables.getStackTraceAsString(e).contains("overlapping/ddd"));
+            assertTrue(Throwables.getStackTraceAsString(e).contains("Central Directory"));
+        }
+    }
+
+    @Test
+    public void readFileWithOffsetBeyondFileEnd() throws Exception {
+        File myZip = ZipTestUtils.cloneRsrc("entry-outside-file.zip", mTemporaryFolder);
+        try (ZFile zf = new ZFile(myZip)) {
+            fail();
+        } catch (IOException e) {
+            assertTrue(Throwables.getStackTraceAsString(e).contains("entry-outside-file/foo"));
+            assertTrue(Throwables.getStackTraceAsString(e).contains("EOF"));
+        }
+    }
 }
diff --git a/src/test/resources/testData/packaging/entry-outside-file.zip b/src/test/resources/testData/packaging/entry-outside-file.zip
new file mode 100644
index 0000000..ffd6be9
--- /dev/null
+++ b/src/test/resources/testData/packaging/entry-outside-file.zip
diff --git a/src/test/resources/testData/packaging/overlapping.zip b/src/test/resources/testData/packaging/overlapping.zip
new file mode 100644
index 0000000..7f6144c
--- /dev/null
+++ b/src/test/resources/testData/packaging/overlapping.zip
diff --git a/src/test/resources/testData/packaging/overlapping2.zip b/src/test/resources/testData/packaging/overlapping2.zip
new file mode 100644
index 0000000..eecefa9
--- /dev/null
+++ b/src/test/resources/testData/packaging/overlapping2.zip