Make SingleJar remove the extended timestamp (0x5455) and InfoZip New Unix (0x7875) extra fields.

This is important so that the output .zip files are actually hermetic. In particular, this caused problems with running the tests for singlejar on OS X Yosemite.

Also add some defensive copying as a drive-by cleanup.

--
MOS_MIGRATED_REVID=102026257

4 files changed, 30 insertions, 7 deletions

diff --git a/src/java_tools/singlejar/java/com/google/devtools/build/zip/ExtraDataList.java b/src/java_tools/singlejar/java/com/google/devtools/build/zip/ExtraDataList.java
index b3cd252e2e..2f7e891ee3 100644
--- a/src/java_tools/singlejar/java/com/google/devtools/build/zip/ExtraDataList.java
+++ b/src/java_tools/singlejar/java/com/google/devtools/build/zip/ExtraDataList.java
@@ -25,6 +25,11 @@ import java.util.LinkedHashMap;
  * underlying buffer. 
  */
 public class ExtraDataList {
+  public static final short ZIP64 = 0x0001;
+  public static final short EXTENDED_TIMESTAMP = 0x5455;
+  // Some documentation says that this is actually 0x7855, but zip files do not seem to corroborate
+  // this
+  public static final short INFOZIP_UNIX_NEW = 0x7875;
   private final LinkedHashMap<Short, ExtraData> entries;
 
   /**
@@ -36,6 +41,11 @@ public class ExtraDataList {
     entries = new LinkedHashMap<>();
   }
 
+  public ExtraDataList(ExtraDataList other) {
+    this.entries = new LinkedHashMap<>();
+    this.entries.putAll(other.entries);
+  }
+
   /**
    * Creates an extra data list from the given extra data records.
    *
diff --git a/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipFileEntry.java b/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipFileEntry.java
index f4e08ab73c..ad47671550 100644
--- a/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipFileEntry.java
+++ b/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipFileEntry.java
@@ -149,7 +149,7 @@ public final class ZipFileEntry {
     this.internalAttributes = e.getInternalAttributes();
     this.externalAttributes = e.getExternalAttributes();
     this.localHeaderOffset = e.getLocalHeaderOffset();
-    this.extra = e.getExtra();
+    this.extra = new ExtraDataList(e.getExtra());
     this.comment = e.getComment();
     this.featureSet = EnumSet.copyOf(e.getFeatureSet());
   }
diff --git a/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipUtil.java b/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipUtil.java
index 978bcfb84c..82211f16fc 100644
--- a/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipUtil.java
+++ b/src/java_tools/singlejar/java/com/google/devtools/build/zip/ZipUtil.java
@@ -248,6 +248,10 @@ public class ZipUtil {
     static byte[] create(ZipFileEntry entry, ZipFileData file, boolean allowZip64)
         throws IOException {
       byte[] name = entry.getName().getBytes(file.getCharset());
+
+      // We don't do a defensive copy here so that later, when we write the central directory entry,
+      // the changes we make here take effect.
+      // TODO(bazel-team): This seems like a bug. Investigate.
       ExtraDataList extra = entry.getExtra();
 
       EnumSet<Feature> features = entry.getFeatureSet();
@@ -286,6 +290,9 @@ public class ZipUtil {
         extra.remove((short) 0x0001);
       }
 
+      extra.remove(ExtraDataList.EXTENDED_TIMESTAMP);
+      extra.remove(ExtraDataList.INFOZIP_UNIX_NEW);
+
       byte[] buf = new byte[FIXED_DATA_SIZE + name.length + extra.getLength()];
       intToLittleEndian(buf, SIGNATURE_OFFSET, SIGNATURE);
       shortToLittleEndian(buf, VERSION_OFFSET, entry.getVersionNeeded());
@@ -439,23 +446,28 @@ public class ZipUtil {
         buf = new byte[FIXED_DATA_SIZE];
       }
 
+      ExtraDataList extra = new ExtraDataList(entry.getExtra());
       if (allowZip64) {
         addZip64Extra(entry);
       } else {
-        entry.getExtra().remove((short) 0x0001);
+        extra.remove((short) 0x0001);
       }
+
+      extra.remove(ExtraDataList.EXTENDED_TIMESTAMP);
+      extra.remove(ExtraDataList.INFOZIP_UNIX_NEW);
+
       byte[] name = entry.getName().getBytes(file.getCharset());
-      byte[] extra = entry.getExtra().getBytes();
+      byte[] extraBytes = extra.getBytes();
       byte[] comment = entry.getComment() != null
           ? entry.getComment().getBytes(file.getCharset()) : new byte[]{};
 
-      fillFixedSizeData(buf, entry, name.length, extra.length, comment.length, allowZip64);
+      fillFixedSizeData(buf, entry, name.length, extraBytes.length, comment.length, allowZip64);
       stream.write(buf, 0, FIXED_DATA_SIZE);
       stream.write(name);
-      stream.write(extra);
+      stream.write(extraBytes);
       stream.write(comment);
 
-      return FIXED_DATA_SIZE + name.length + extra.length + comment.length;
+      return FIXED_DATA_SIZE + name.length + extraBytes.length + comment.length;
     }
 
     /**
diff --git a/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java b/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java
index 94cb25f480..a2775cad5c 100644
--- a/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java
+++ b/src/java_tools/singlejar/javatests/com/google/devtools/build/singlejar/ZipCombinerTest.java
@@ -769,7 +769,8 @@ public class ZipCombinerTest {
     assertThat(x.getCompressedSize()).isEqualTo(y.getCompressedSize());
     assertThat(x.getCrc()).isEqualTo(y.getCrc());
     assertThat(x.getExternalAttributes()).isEqualTo(y.getExternalAttributes());
-    assertThat(x.getExtra().getBytes()).isEqualTo(y.getExtra().getBytes());
+    // The JDK adds different extra data to zip files on different platforms, so we don't compare
+    // the extra data.
     assertThat(x.getInternalAttributes()).isEqualTo(y.getInternalAttributes());
     assertThat(x.getMethod()).isEqualTo(y.getMethod());
     assertThat(x.getName()).isEqualTo(y.getName());