From 4d053e15d8fa58a87b1b1c5da55efb456e547e5e Mon Sep 17 00:00:00 2001 From: mathieufortin01 Date: Sun, 13 Sep 2020 14:01:11 -0700 Subject: [PATCH] Fix signed jar performance issues Update Spring Boot nested JarFile support to improve the performance of signed jars. Prior to this commit, `certificates` and `codeSigners` were read by streaming the entire jar whenever the existing values were `null`. Unfortunately, the contract for `getCertificates` and get `getCodeSigners` states that `null` is a valid return value. This meant that full jar streaming would occur whenever either method was called on an entry that had no result. The problem was further exacerbated by the fact that entries might not be cached. See gh-19041 --- .../springframework/boot/loader/jar/JarEntry.java | 12 ++++++++---- .../springframework/boot/loader/jar/JarFile.java | 14 +++++--------- .../boot/loader/jar/JarFileEntries.java | 2 +- .../boot/loader/jar/JarFileTests.java | 14 +++++++++++++- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java index aed07d1bc7..77cc68d209 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarEntry.java @@ -85,16 +85,16 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { @Override public Certificate[] getCertificates() { - if (this.jarFile.isSigned() && this.certificates == null) { - this.jarFile.setupEntryCertificates(this); + if (this.jarFile.isSigned() && this.certificates == null && isSignable()) { + this.jarFile.setupEntryCertificates(); } return this.certificates; } @Override public CodeSigner[] getCodeSigners() { - if (this.jarFile.isSigned() && this.codeSigners == null) { - this.jarFile.setupEntryCertificates(this); + if (this.jarFile.isSigned() && this.codeSigners == null && isSignable()) { + this.jarFile.setupEntryCertificates(); } return this.codeSigners; } @@ -109,4 +109,8 @@ class JarEntry extends java.util.jar.JarEntry implements FileHeader { return this.localHeaderOffset; } + private boolean isSignable() { + return !isDirectory() && !getName().startsWith("META-INF"); + } + } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java index 2649588f6e..6d8d58b6bd 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFile.java @@ -387,19 +387,15 @@ public class JarFile extends java.util.jar.JarFile { return this.signed; } - void setupEntryCertificates(JarEntry entry) { + void setupEntryCertificates() { // Fallback to JarInputStream to obtain certificates, not fast but hopefully not // happening that often. try { - try (JarInputStream inputStream = new JarInputStream(getData().getInputStream())) { - java.util.jar.JarEntry certEntry = inputStream.getNextJarEntry(); - while (certEntry != null) { - inputStream.closeEntry(); - if (entry.getName().equals(certEntry.getName())) { - setCertificates(entry, certEntry); - } + 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); - certEntry = inputStream.getNextJarEntry(); } } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java index 1c983663fd..152f837af1 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/jar/JarFileEntries.java @@ -363,7 +363,7 @@ class JarFileEntries implements CentralDirectoryVisitor, Iterable { } int entryIndex = JarFileEntries.this.positions[this.index]; this.index++; - return getEntry(entryIndex, JarEntry.class, false, null); + return getEntry(entryIndex, JarEntry.class, true, null); } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java index 3459f76c18..74c39c7cea 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/JarFileTests.java @@ -26,7 +26,9 @@ import java.net.URL; 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; @@ -387,17 +389,27 @@ public class JarFileTests { java.util.jar.JarFile jarFile = new JarFile(new File(signedJarFile)); jarFile.getManifest(); Enumeration 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(jarEntry.getCertificates()).isNotNull(); + assertThat(certs).isNotNull(); } } jarFile.close(); + + // 3 seconds is still quite long, but low enough to catch most problems + assertThat(ChronoUnit.SECONDS.between(start, Instant.now())).isLessThanOrEqualTo(3L); } @Test