8244624: Improve handling of JarFile META-INF resources

Reviewed-by: lancea, weijun, martin

src/java.base/share/classes/java/util/jar/JarFile.java

@@ -715,22 +715,15 @@ public class JarFile extends ZipFile {
         }
 
         if (verify) {
-            String[] names = JUZFA.getMetaInfEntryNames(this);
+            // Gets the manifest name, but only if there are
-            if (names != null) {
+            // signature-related files. If so we can assume
-                for (String nameLower : names) {
-                    String name = nameLower.toUpperCase(Locale.ENGLISH);
-                    if (name.endsWith(".DSA") ||
-                        name.endsWith(".RSA") ||
-                        name.endsWith(".EC") ||
-                        name.endsWith(".SF")) {
-                        // Assume since we found a signature-related file
             // that the jar is signed and that we therefore
             // need a JarVerifier and Manifest
+            String name = JUZFA.getManifestName(this, true);
+            if (name != null) {
                 getManifest();
                 return;
             }
-                }
-            }
             // No signature-related files; don't instantiate a
             // verifier
             verify = false;
@@ -746,12 +739,8 @@ public class JarFile extends ZipFile {
 
         // Verify "META-INF/" entries...
         try {
-            String[] names = JUZFA.getMetaInfEntryNames(this);
+            List<String> names = JUZFA.getManifestAndSignatureRelatedFiles(this);
-            if (names != null) {
             for (String name : names) {
-                    String uname = name.toUpperCase(Locale.ENGLISH);
-                    if (MANIFEST_NAME.equals(uname)
-                            || SignatureFileVerifier.isBlockOrSF(uname)) {
                 JarEntry e = getJarEntry(name);
                 if (e == null) {
                     throw new JarException("corrupted jar file");
@@ -767,9 +756,7 @@ public class JarFile extends ZipFile {
                     jv.update(-1, null, 0, 0, mev);
                 }
             }
-                }
+        } catch (IOException | IllegalArgumentException ex) {
-            }
-        } catch (IOException ex) {
             // if we had an error parsing any blocks, just
             // treat the jar file as being unsigned
             jv = null;
@@ -935,23 +922,13 @@ public class JarFile extends ZipFile {
 
     private JarEntry getManEntry() {
         if (manEntry == null) {
-            // First look up manifest entry using standard name
+            // The manifest entry position is resolved during
-            JarEntry manEntry = getEntry0(MANIFEST_NAME);
+            // initialization
-            if (manEntry == null) {
+            String name = JUZFA.getManifestName(this, false);
-                // If not found, then iterate through all the "META-INF/"
+            if (name != null) {
-                // entries to find a match.
+                this.manEntry = getEntry0(name);
-                String[] names = JUZFA.getMetaInfEntryNames(this);
-                if (names != null) {
-                    for (String name : names) {
-                        if (MANIFEST_NAME.equals(name.toUpperCase(Locale.ENGLISH))) {
-                            manEntry = getEntry0(name);
-                            break;
             }
         }
-                }
-            }
-            this.manEntry = manEntry;
-        }
         return manEntry;
     }
 
@@ -1213,7 +1190,7 @@ public class JarFile extends ZipFile {
         ensureInitialization();
         if (jv != null) {
             if (jv.eagerValidation) {
-                CodeSource cs = null;
+                CodeSource cs;
                 JarEntry je = getJarEntry(name);
                 if (je != null) {
                     cs = jv.getCodeSource(url, this, je);

src/java.base/share/classes/java/util/zip/ZipFile.java

@@ -45,6 +45,8 @@ import java.util.Deque;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.List;
+import java.util.Locale;
 import java.util.Objects;
 import java.util.NoSuchElementException;
 import java.util.Set;
@@ -66,6 +68,7 @@ import jdk.internal.perf.PerfCounter;
 import jdk.internal.ref.CleanerFactory;
 import jdk.internal.vm.annotation.Stable;
 import sun.nio.cs.UTF_8;
+import sun.security.util.SignatureFileVerifier;
 
 import static java.util.zip.ZipConstants64.*;
 import static java.util.zip.ZipUtils.*;
@@ -1010,30 +1013,50 @@ public class ZipFile implements ZipConstants, Closeable {
     }
 
     /**
-     * Returns the names of all non-directory entries that begin with
+     * Returns the names of the META-INF/MANIFEST.MF entry - if exists -
-     * "META-INF/" (case ignored). This method is used in JarFile, via
+     * and any signature-related files under META-INF. This method is used in
-     * SharedSecrets, as an optimization when looking up manifest and
+     * JarFile, via SharedSecrets, as an optimization.
-     * signature file entries. Returns null if no entries were found.
      */
-    private String[] getMetaInfEntryNames() {
+    private List<String> getManifestAndSignatureRelatedFiles() {
         synchronized (this) {
             ensureOpen();
             Source zsrc = res.zsrc;
-            if (zsrc.metanames == null) {
+            int[] metanames = zsrc.signatureMetaNames;
+            List<String> files = null;
+            if (zsrc.manifestPos >= 0) {
+                files = new ArrayList<>();
+                files.add(getEntryName(zsrc.manifestPos));
+            }
+            if (metanames != null) {
+                if (files == null) {
+                    files = new ArrayList<>();
+                }
+                for (int i = 0; i < metanames.length; i++) {
+                    files.add(getEntryName(metanames[i]));
+                }
+            }
+            return files == null ? List.of() : files;
+        }
+    }
+
+    /**
+     * Returns the name of the META-INF/MANIFEST.MF entry, ignoring
+     * case. If {@code onlyIfSignatureRelatedFiles} is true, we only return the
+     * manifest if there is also at least one signature-related file.
+     * This method is used in JarFile, via SharedSecrets, as an optimization
+     * when looking up the manifest file.
+     */
+    private String getManifestName(boolean onlyIfSignatureRelatedFiles) {
+        synchronized (this) {
+            ensureOpen();
+            Source zsrc = res.zsrc;
+            int pos = zsrc.manifestPos;
+            if (pos >= 0 && (!onlyIfSignatureRelatedFiles || zsrc.signatureMetaNames != null)) {
+                return getEntryName(pos);
+            }
+        }
         return null;
     }
-            String[] names = new String[zsrc.metanames.length];
-            byte[] cen = zsrc.cen;
-            for (int i = 0; i < names.length; i++) {
-                int pos = zsrc.metanames[i];
-                // This will only be invoked on JarFile, which is guaranteed
-                // to use (or be compatible with) UTF-8 encoding.
-                names[i] = new String(cen, pos + CENHDR, CENNAM(cen, pos),
-                                      UTF_8.INSTANCE);
-            }
-            return names;
-        }
-    }
 
     /**
      * Returns the versions for which there exists a non-directory
@@ -1059,8 +1082,12 @@ public class ZipFile implements ZipConstants, Closeable {
                     return zip.res.zsrc.startsWithLoc;
                 }
                 @Override
-                public String[] getMetaInfEntryNames(JarFile jar) {
+                public List<String> getManifestAndSignatureRelatedFiles(JarFile jar) {
-                    return ((ZipFile)jar).getMetaInfEntryNames();
+                    return ((ZipFile)jar).getManifestAndSignatureRelatedFiles();
+                }
+                @Override
+                public String getManifestName(JarFile jar, boolean onlyIfHasSignatureRelatedFiles) {
+                    return ((ZipFile)jar).getManifestName(onlyIfHasSignatureRelatedFiles);
                 }
                 @Override
                 public int[] getMetaInfVersions(JarFile jar) {
@@ -1105,7 +1132,8 @@ public class ZipFile implements ZipConstants, Closeable {
         private long locpos;                 // position of first LOC header (usually 0)
         private byte[] comment;              // zip file comment
                                              // list of meta entries in META-INF dir
-        private int[] metanames;
+        private int   manifestPos = -1;      // position of the META-INF/MANIFEST.MF, if exists
+        private int[] signatureMetaNames;    // positions of signature related entries, if such exist
         private int[] metaVersions;          // list of unique versions found in META-INF/versions/
         private final boolean startsWithLoc; // true, if zip file starts with LOCSIG (usually true)
 
@@ -1254,7 +1282,8 @@ public class ZipFile implements ZipConstants, Closeable {
             cen = null;
             entries = null;
             table = null;
-            metanames = null;
+            manifestPos = -1;
+            signatureMetaNames = null;
             metaVersions = EMPTY_META_VERSIONS;
         }
 
@@ -1438,7 +1467,7 @@ public class ZipFile implements ZipConstants, Closeable {
             int next;
 
             // list for all meta entries
-            ArrayList<Integer> metanamesList = null;
+            ArrayList<Integer> signatureNames = null;
             // Set of all version numbers seen in META-INF/versions/
             Set<Integer> metaVersionsSet = null;
 
@@ -1476,9 +1505,16 @@ public class ZipFile implements ZipConstants, Closeable {
                 idx = addEntry(idx, hash, next, pos);
                 // Adds name to metanames.
                 if (isMetaName(cen, entryPos, nlen)) {
-                    if (metanamesList == null)
+                    // nlen is at least META_INF_LENGTH
-                        metanamesList = new ArrayList<>(4);
+                    if (isManifestName(cen, entryPos + META_INF_LENGTH,
-                    metanamesList.add(pos);
+                            nlen - META_INF_LENGTH)) {
+                        manifestPos = pos;
+                    } else {
+                        if (isSignatureRelated(cen, entryPos, nlen)) {
+                            if (signatureNames == null)
+                                signatureNames = new ArrayList<>(4);
+                            signatureNames.add(pos);
+                        }
 
                         // If this is a versioned entry, parse the version
                         // and store it for later. This optimizes lookup
@@ -1491,16 +1527,18 @@ public class ZipFile implements ZipConstants, Closeable {
                             metaVersionsSet.add(version);
                         }
                     }
+                }
                 // skip ext and comment
                 pos = entryPos + nlen + elen + clen;
                 entryPos = pos + CENHDR;
                 i++;
             }
             total = i;
-            if (metanamesList != null) {
+            if (signatureNames != null) {
-                metanames = new int[metanamesList.size()];
+                int len = signatureNames.size();
-                for (int j = 0, len = metanames.length; j < len; j++) {
+                signatureMetaNames = new int[len];
-                    metanames[j] = metanamesList.get(j);
+                for (int j = 0; j < len; j++) {
+                    signatureMetaNames[j] = signatureNames.get(j);
                 }
             }
             if (metaVersionsSet != null) {
@@ -1580,7 +1618,8 @@ public class ZipFile implements ZipConstants, Closeable {
          * beginning with "META-INF/", disregarding ASCII case.
          */
         private static boolean isMetaName(byte[] name, int off, int len) {
-            // Use the "oldest ASCII trick in the book"
+            // Use the "oldest ASCII trick in the book":
+            // ch | 0x20 == Character.toLowerCase(ch)
             return len > META_INF_LENGTH       // "META-INF/".length()
                 && name[off + len - 1] != '/'  // non-directory
                 && (name[off++] | 0x20) == 'm'
@@ -1594,6 +1633,52 @@ public class ZipFile implements ZipConstants, Closeable {
                 && (name[off]         ) == '/';
         }
 
+        /*
+         * Check if the bytes represents a name equals to MANIFEST.MF
+         */
+        private static boolean isManifestName(byte[] name, int off, int len) {
+            return (len == 11 // "MANIFEST.MF".length()
+                    && (name[off++] | 0x20) == 'm'
+                    && (name[off++] | 0x20) == 'a'
+                    && (name[off++] | 0x20) == 'n'
+                    && (name[off++] | 0x20) == 'i'
+                    && (name[off++] | 0x20) == 'f'
+                    && (name[off++] | 0x20) == 'e'
+                    && (name[off++] | 0x20) == 's'
+                    && (name[off++] | 0x20) == 't'
+                    && (name[off++]       ) == '.'
+                    && (name[off++] | 0x20) == 'm'
+                    && (name[off]   | 0x20) == 'f');
+        }
+
+        private static boolean isSignatureRelated(byte[] name, int off, int len) {
+            // Only called when isMetaName(name, off, len) is true, which means
+            // len is at least META_INF_LENGTH
+            // assert isMetaName(name, off, len)
+            boolean signatureRelated = false;
+            if (name[off + len - 3] == '.') {
+                // Check if entry ends with .EC and .SF
+                int b1 = name[off + len - 2] | 0x20;
+                int b2 = name[off + len - 1] | 0x20;
+                if ((b1 == 'e' && b2 == 'c') || (b1 == 's' && b2 == 'f')) {
+                    signatureRelated = true;
+                }
+            } else if (name[off + len - 4] == '.') {
+                // Check if entry ends with .DSA and .RSA
+                int b1 = name[off + len - 3] | 0x20;
+                int b2 = name[off + len - 2] | 0x20;
+                int b3 = name[off + len - 1] | 0x20;
+                if ((b1 == 'r' || b1 == 'd') && b2 == 's' && b3 == 'a') {
+                    signatureRelated = true;
+                }
+            }
+            // Above logic must match SignatureFileVerifier.isBlockOrSF
+            assert(signatureRelated == SignatureFileVerifier
+                .isBlockOrSF(new String(name, off, len, UTF_8.INSTANCE)
+                    .toUpperCase(Locale.ENGLISH)));
+            return signatureRelated;
+        }
+
         /*
          * If the bytes represents a non-directory name beginning
          * with "versions/", continuing with a positive integer,

src/java.base/share/classes/jdk/internal/access/JavaUtilZipFileAccess.java

@@ -26,6 +26,7 @@
 package jdk.internal.access;
 
 import java.util.Enumeration;
+import java.util.List;
 import java.util.function.Function;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
@@ -34,7 +35,8 @@ import java.util.zip.ZipFile;
 
 public interface JavaUtilZipFileAccess {
     public boolean startsWithLocHeader(ZipFile zip);
-    public String[] getMetaInfEntryNames(JarFile zip);
+    public List<String> getManifestAndSignatureRelatedFiles(JarFile zip);
+    public String getManifestName(JarFile zip, boolean onlyIfSignatureRelatedFiles);
     public int[] getMetaInfVersions(JarFile zip);
     public JarEntry getEntry(ZipFile zip, String name, Function<String, JarEntry> func);
     public Enumeration<JarEntry> entries(ZipFile zip, Function<String, JarEntry> func);

src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java

@@ -175,6 +175,7 @@ public class SignatureFileVerifier {
      *          Signature File or PKCS7 block file name
      */
     public static boolean isBlockOrSF(String s) {
+        // Note: keep this in sync with j.u.z.ZipFile.Source#isSignatureRelated
         // we currently only support DSA and RSA PKCS7 blocks
         return s.endsWith(".SF")
             || s.endsWith(".DSA")

test/micro/org/openjdk/bench/java/util/jar/JarFileMeta.java

@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package org.openjdk.bench.java.util.jar;
+
+import org.openjdk.jmh.annotations.*;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.util.Random;
+import java.util.concurrent.TimeUnit;
+import java.util.jar.JarFile;
+import java.util.jar.JarOutputStream;
+import java.util.jar.Manifest;
+import java.util.zip.ZipEntry;
+
+/**
+ * Simple benchmark measuring cost of various operations relating to jar
+ * meta-inf and manifests, especially costs incurred during opening of the
+ * file, and when opening an input stream (which runs
+ * JarFile.maybeInstantiateVerifier)
+ *
+ * Before JDK-8244624:
+ * Benchmark                          (size)  Mode  Cnt       Score    Error   Units
+ * getManifestFromJarWithManifest       1024  avgt    5     232.437   31.535   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  206410.627    2.833    B/op
+ * getStreamFromJarWithManifest         1024  avgt    5     277.696   32.078   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  250454.252    2.452    B/op
+ * getStreamFromJarWithNoManifest       1024  avgt    5     312.432   58.663   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  301802.644   13.276    B/op
+ * getStreamFromJarWithSignatureFiles   1024  avgt    5     315.752   55.048   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  305354.934   14.093    B/op
+ *
+ * With JDK-8244624:
+ * Benchmark                          (size)  Mode  Cnt       Score    Error   Units
+ * getManifestFromJarWithManifest       1024  avgt    5     215.242   32.085   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  196609.220   14.788    B/op
+ * getStreamFromJarWithManifest         1024  avgt    5     216.435   10.876   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  187960.147    9.026    B/op
+ * getStreamFromJarWithNoManifest       1024  avgt    5     204.256   25.744   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  186784.347    1.841    B/op
+ * getStreamFromJarWithSignatureFiles   1024  avgt    5     247.972   38.574   us/op
+ *   gc.alloc.rate.norm                 1024  avgt    5  211577.268   15.109    B/op
+ */
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.MICROSECONDS)
+@State(Scope.Thread)
+@Warmup(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
+@Measurement(iterations = 5, time = 1000, timeUnit = TimeUnit.MILLISECONDS)
+@Fork(3)
+public class JarFileMeta {
+
+    @Param({"512", "1024"})
+    private int size;
+
+    public File jarManifest;
+    public File jarNoManifest;
+    public File jarManifestSignature;
+
+    public int index = 0;
+
+    @Setup(Level.Trial)
+    public void beforeRun() throws IOException {
+        jarNoManifest = createJar(false, false);
+        jarManifest = createJar(true, false);
+        jarManifestSignature = createJar(true, true);
+    }
+
+    private File createJar(boolean manifest, boolean signatureFiles) throws IOException {
+        // Create a test Zip file with the number of entries.
+        File tempFile = Files.createTempFile("jar-micro", ".jar").toFile();
+        tempFile.deleteOnExit();
+
+        try (FileOutputStream fos = new FileOutputStream(tempFile);
+             JarOutputStream zos = manifest
+                     ? new JarOutputStream(fos, new Manifest())
+                     : new JarOutputStream(fos)) {
+
+            // Always add this
+            zos.putNextEntry(new ZipEntry("README"));
+
+            Random random = new Random(4711);
+            for (int i = 0; i < size; i++) {
+                String ename = "directory-" + (random.nextInt(90000) + 10000) + "-" + i + "/";
+                if (random.nextInt(100) > 70) {
+                    ename = "META-INF/" + ename;
+
+                }
+                zos.putNextEntry(new ZipEntry(ename));
+
+                ename += "entry-"  + (random.nextInt(90000) + 10000)  + "-" + i;
+                if (signatureFiles && random.nextInt(100) > 95) {
+                    ename += ".DSA";
+                }
+                zos.putNextEntry(new ZipEntry(ename));
+            }
+        }
+        return tempFile;
+    }
+
+    private InputStream openGetStreamAndClose(File file) throws IOException {
+        JarFile jf = new JarFile(file);
+        InputStream is = jf.getInputStream(jf.getEntry("README"));
+        jf.close();
+        // we'll never actually read from the closed stream, rather just
+        // return it to avoid DCE
+        return is;
+    }
+
+    @Benchmark
+    public InputStream getStreamFromJarWithManifest() throws IOException {
+        return openGetStreamAndClose(jarManifest);
+    }
+
+    @Benchmark
+    public InputStream getStreamFromJarWithNoManifest() throws IOException {
+        return openGetStreamAndClose(jarNoManifest);
+    }
+
+    @Benchmark
+    public InputStream getStreamFromJarWithSignatureFiles() throws IOException {
+        return openGetStreamAndClose(jarManifestSignature);
+    }
+
+    @Benchmark
+    public Manifest getManifestFromJarWithManifest() throws IOException {
+        JarFile jf = new JarFile(jarManifest);
+        Manifest mf = jf.getManifest();
+        jf.close();
+        return mf;
+    }
+}