Polish 'Fix signed jar performance issues'

Update the performance improvements to push certificate loading
and storage into the `JarFileEntries` class. This allows us to
keep certificates without needing to cache all entry data. We
now also keep certificates and code signers in a dedicated class
which is set whenever the full jar stream as been read, even if
the contained values are `null`. The logic that assumes META-INF
entries are not signed has been removed in favor of delegating to
the streamed entry results.

See gh-19041

Author: philwebb

spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2019 the original author or authors.
+ * Copyright 2012-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -32,20 +32,21 @@
  */
 class JarEntry extends java.util.jar.JarEntry implements FileHeader {
 
+	private final int index;
+
 	private final AsciiBytes name;
 
 	private final AsciiBytes headerName;
 
-	private Certificate[] certificates;
-
-	private CodeSigner[] codeSigners;
-
 	private final JarFile jarFile;
 
 	private long localHeaderOffset;
 
-	JarEntry(JarFile jarFile, CentralDirectoryFileHeader header, AsciiBytes nameAlias) {
+	private volatile JarEntryCertification certification;
+
+	JarEntry(JarFile jarFile, int index, CentralDirectoryFileHeader header, AsciiBytes nameAlias) {
 		super((nameAlias != null) ? nameAlias.toString() : header.getName().toString());
+		this.index = index;
 		this.name = (nameAlias != null) ? nameAlias : header.getName();
 		this.headerName = header.getName();
 		this.jarFile = jarFile;
@@ -59,6 +60,10 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader {
 		setExtra(header.getExtra());
 	}
 
+	int getIndex() {
+		return this.index;
+	}
+
 	AsciiBytes getAsciiBytesName() {
 		return this.name;
 	}
@@ -85,32 +90,29 @@ public Attributes getAttributes() throws IOException {
 
 	@Override
 	public Certificate[] getCertificates() {
-		if (this.jarFile.isSigned() && this.certificates == null && isSignable()) {
-			this.jarFile.setupEntryCertificates();
-		}
-		return this.certificates;
+		return getCertification().getCertificates();
 	}
 
 	@Override
 	public CodeSigner[] getCodeSigners() {
-		if (this.jarFile.isSigned() && this.codeSigners == null && isSignable()) {
-			this.jarFile.setupEntryCertificates();
-		}
-		return this.codeSigners;
+		return getCertification().getCodeSigners();
 	}
 
-	void setCertificates(java.util.jar.JarEntry entry) {
-		this.certificates = entry.getCertificates();
-		this.codeSigners = entry.getCodeSigners();
+	private JarEntryCertification getCertification() {
+		if (!this.jarFile.isSigned()) {
+			return JarEntryCertification.NONE;
+		}
+		JarEntryCertification certification = this.certification;
+		if (certification == null) {
+			certification = this.jarFile.getCertification(this);
+			this.certification = certification;
+		}
+		return certification;
 	}
 
 	@Override
 	public long getLocalHeaderOffset() {
 		return this.localHeaderOffset;
 	}
 
-	private boolean isSignable() {
-		return !isDirectory() && !getName().startsWith("META-INF");
-	}
-
 }

spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntryCertification.java

@@ -0,0 +1,58 @@
+/*
+ * Copyright 2012-2020 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.boot.loader.jar;
+
+import java.security.CodeSigner;
+import java.security.cert.Certificate;
+
+/**
+ * {@link Certificate} and {@link CodeSigner} details for a {@link JarEntry} from a signed
+ * {@link JarFile}.
+ *
+ * @author Phillip Webb
+ */
+class JarEntryCertification {
+
+	static final JarEntryCertification NONE = new JarEntryCertification(null, null);
+
+	private final Certificate[] certificates;
+
+	private final CodeSigner[] codeSigners;
+
+	JarEntryCertification(Certificate[] certificates, CodeSigner[] codeSigners) {
+		this.certificates = certificates;
+		this.codeSigners = codeSigners;
+	}
+
+	Certificate[] getCertificates() {
+		return (this.certificates != null) ? this.certificates.clone() : null;
+	}
+
+	CodeSigner[] getCodeSigners() {
+		return (this.codeSigners != null) ? this.codeSigners.clone() : null;
+	}
+
+	public static JarEntryCertification from(java.util.jar.JarEntry certifiedEntry) {
+		Certificate[] certificates = (certifiedEntry != null) ? certifiedEntry.getCertificates() : null;
+		CodeSigner[] codeSigners = (certifiedEntry != null) ? certifiedEntry.getCodeSigners() : null;
+		if (certificates == null && codeSigners == null) {
+			return NONE;
+		}
+		return new JarEntryCertification(certificates, codeSigners);
+	}
+
+}

spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java

@@ -27,7 +27,6 @@
 import java.util.Enumeration;
 import java.util.Iterator;
 import java.util.function.Supplier;
-import java.util.jar.JarInputStream;
 import java.util.jar.Manifest;
 import java.util.zip.ZipEntry;
 
@@ -387,29 +386,15 @@ boolean isSigned() {
 		return this.signed;
 	}
 
-	void setupEntryCertificates() {
-		// Fallback to JarInputStream to obtain certificates, not fast but hopefully not
-		// happening that often.
+	JarEntryCertification getCertification(JarEntry entry) {
 		try {
-			try (JarInputStream jarStream = new JarInputStream(getData().getInputStream())) {
-				java.util.jar.JarEntry certEntry = null;
-				while ((certEntry = jarStream.getNextJarEntry()) != null) {
-					jarStream.closeEntry();
-					setCertificates(getJarEntry(certEntry.getName()), certEntry);
-				}
-			}
+			return this.entries.getCertification(entry);
 		}
 		catch (IOException ex) {
 			throw new IllegalStateException(ex);
 		}
 	}
 
-	private void setCertificates(JarEntry entry, java.util.jar.JarEntry certEntry) {
-		if (entry != null) {
-			entry.setCertificates(certEntry);
-		}
-	}
-
 	public void clearCache() {
 		this.entries.clearCache();
 	}

spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2019 the original author or authors.
+ * Copyright 2012-2020 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -26,6 +26,7 @@
 import java.util.NoSuchElementException;
 import java.util.jar.Attributes;
 import java.util.jar.Attributes.Name;
+import java.util.jar.JarInputStream;
 import java.util.jar.Manifest;
 import java.util.zip.ZipEntry;
 
@@ -91,14 +92,13 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable<JarEntry> {
 
 	private Boolean multiReleaseJar;
 
+	private JarEntryCertification[] certifications;
+
 	private final Map<Integer, FileHeader> entriesCache = Collections
 			.synchronizedMap(new LinkedHashMap<Integer, FileHeader>(16, 0.75f, true) {
 
 				@Override
 				protected boolean removeEldestEntry(Map.Entry<Integer, FileHeader> eldest) {
-					if (JarFileEntries.this.jarFile.isSigned()) {
-						return false;
-					}
 					return size() >= ENTRY_CACHE_SIZE;
 				}
 
@@ -313,7 +313,7 @@ private <T extends FileHeader> T getEntry(int index, Class<T> type, boolean cach
 			FileHeader entry = (cached != null) ? cached : CentralDirectoryFileHeader
 					.fromRandomAccessData(this.centralDirectoryData, this.centralDirectoryOffsets[index], this.filter);
 			if (CentralDirectoryFileHeader.class.equals(entry.getClass()) && type.equals(JarEntry.class)) {
-				entry = new JarEntry(this.jarFile, (CentralDirectoryFileHeader) entry, nameAlias);
+				entry = new JarEntry(this.jarFile, index, (CentralDirectoryFileHeader) entry, nameAlias);
 			}
 			if (cacheEntry && cached != entry) {
 				this.entriesCache.put(index, entry);
@@ -344,6 +344,41 @@ private AsciiBytes applyFilter(AsciiBytes name) {
 		return (this.filter != null) ? this.filter.apply(name) : name;
 	}
 
+	JarEntryCertification getCertification(JarEntry entry) throws IOException {
+		JarEntryCertification[] certifications = this.certifications;
+		if (certifications == null) {
+			certifications = new JarEntryCertification[this.size];
+			// We fallback to use JarInputStream to obtain the certs. This isn't that
+			// fast, but hopefully doesn't happen too often.
+			try (JarInputStream certifiedJarStream = new JarInputStream(this.jarFile.getData().getInputStream())) {
+				java.util.jar.JarEntry certifiedEntry = null;
+				while ((certifiedEntry = certifiedJarStream.getNextJarEntry()) != null) {
+					int index = getEntryIndex(certifiedEntry.getName());
+					if (index != -1) {
+						certifications[index] = JarEntryCertification.from(certifiedEntry);
+					}
+					certifiedJarStream.closeEntry();
+				}
+			}
+			this.certifications = certifications;
+		}
+		JarEntryCertification certification = certifications[entry.getIndex()];
+		return (certification != null) ? certification : JarEntryCertification.NONE;
+	}
+
+	private int getEntryIndex(CharSequence name) {
+		int hashCode = AsciiBytes.hashCode(name);
+		int index = getFirstIndex(hashCode);
+		while (index >= 0 && index < this.size && this.hashCodes[index] == hashCode) {
+			CentralDirectoryFileHeader candidate = getEntry(index, CentralDirectoryFileHeader.class, false, null);
+			if (candidate.hasName(name, NO_SUFFIX)) {
+				return index;
+			}
+			index++;
+		}
+		return -1;
+	}
+
 	/**
 	 * Iterator for contained entries.
 	 */
@@ -363,7 +398,7 @@ public JarEntry next() {
 			}
 			int entryIndex = JarFileEntries.this.positions[this.index];
 			this.index++;
-			return getEntry(entryIndex, JarEntry.class, true, null);
+			return getEntry(entryIndex, JarEntry.class, false, null);
 		}
 
 	}

spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java

@@ -26,9 +26,7 @@
 import java.net.URLClassLoader;
 import java.nio.charset.Charset;
 import java.nio.file.attribute.FileTime;
-import java.security.cert.Certificate;
 import java.time.Instant;
-import java.time.temporal.ChronoUnit;
 import java.util.Enumeration;
 import java.util.jar.JarEntry;
 import java.util.jar.JarInputStream;
@@ -46,6 +44,7 @@
 import org.springframework.boot.loader.data.RandomAccessDataFile;
 import org.springframework.test.util.ReflectionTestUtils;
 import org.springframework.util.FileCopyUtils;
+import org.springframework.util.StopWatch;
 import org.springframework.util.StreamUtils;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -377,39 +376,33 @@ public void sensibleToString() throws Exception {
 
 	@Test
 	public void verifySignedJar() throws Exception {
-		String classpath = System.getProperty("java.class.path");
-		String[] entries = classpath.split(System.getProperty("path.separator"));
-		String signedJarFile = null;
-		for (String entry : entries) {
-			if (entry.contains("bcprov")) {
-				signedJarFile = entry;
+		File signedJarFile = getSignedJarFile();
+		assertThat(signedJarFile).exists();
+		try (java.util.jar.JarFile expected = new java.util.jar.JarFile(signedJarFile)) {
+			try (JarFile actual = new JarFile(signedJarFile)) {
+				StopWatch stopWatch = new StopWatch();
+				Enumeration<JarEntry> actualEntries = actual.entries();
+				while (actualEntries.hasMoreElements()) {
+					JarEntry actualEntry = actualEntries.nextElement();
+					java.util.jar.JarEntry expectedEntry = expected.getJarEntry(actualEntry.getName());
+					assertThat(actualEntry.getCertificates()).as(actualEntry.getName())
+							.isEqualTo(expectedEntry.getCertificates());
+					assertThat(actualEntry.getCodeSigners()).as(actualEntry.getName())
+							.isEqualTo(expectedEntry.getCodeSigners());
+				}
+				assertThat(stopWatch.getTotalTimeSeconds()).isLessThan(3.0);
 			}
 		}
-		assertThat(signedJarFile).isNotNull();
-		java.util.jar.JarFile jarFile = new JarFile(new File(signedJarFile));
-		jarFile.getManifest();
-		Enumeration<JarEntry> jarEntries = jarFile.entries();
-
-		// Make sure this whole certificates routine runs in an acceptable time (few
-		// seconds at most)
-		// Some signed jars took from 30s to 5 min depending on the implementation.
-		Instant start = Instant.now();
-		while (jarEntries.hasMoreElements()) {
-			JarEntry jarEntry = jarEntries.nextElement();
-			InputStream inputStream = jarFile.getInputStream(jarEntry);
-			inputStream.skip(Long.MAX_VALUE);
-			inputStream.close();
-
-			Certificate[] certs = jarEntry.getCertificates();
-			if (!jarEntry.getName().startsWith("META-INF") && !jarEntry.isDirectory()
-					&& !jarEntry.getName().endsWith("TigerDigest.class")) {
-				assertThat(certs).isNotNull();
+	}
+
+	private File getSignedJarFile() {
+		String[] entries = System.getProperty("java.class.path").split(System.getProperty("path.separator"));
+		for (String entry : entries) {
+			if (entry.contains("bcprov")) {
+				return new File(entry);
 			}
 		}
-		jarFile.close();
-
-		// 3 seconds is still quite long, but low enough to catch most problems
-		assertThat(ChronoUnit.SECONDS.between(start, Instant.now())).isLessThanOrEqualTo(3L);
+		return null;
 	}
 
 	@Test