On Wed, 18 Feb 2026 12:38:34 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which addresses a regression that >> was introduced after the integration of >> https://bugs.openjdk.org/browse/JDK-8377338? >> >> As noted in https://bugs.openjdk.org/browse/JDK-8378003, after the change in >> JDK-8377338, `JarURLConnection.getCertificates()` and `getCodeSigners()` >> started incorrectly returning null for JAR entries in signed JAR files. >> >> The original change in JDK-8377338 removed the overridden >> `getCertificates()` and `getCodeSigners()` methods from >> `URLJarFile$URLJarFileEntry`. When doing that I had verified that the >> `getCertificates()` and `getCodeSigners()` that would get now invoked on >> `java.util.jar.JarEntry` (due to the removal of these overridden methods) >> were indeed returning the certificates and codesigners that belong to the >> underlying `JarEntry` `je`. I was assured of this because the `certs` and >> `signers` fields were captured in the constructor of `JarEntry` constructor: >> >> >> public JarEntry(JarEntry je) { >> this((ZipEntry)je); >> this.attr = je.attr; >> this.certs = je.certs; >> this.signers = je.signers; >> } >> >> and then JarEntry.getCertificates() and getCodeSigners() did this: >> >> >> public Certificate[] getCertificates() { >> return certs == null ? null : certs.clone(); >> } >> >> public CodeSigner[] getCodeSigners() { >> return signers == null ? null : signers.clone(); >> } >> >> So removal of the overrides appeared harmless. I however missed the fact >> that some JarEntry implementations like `java.util.jar.JarFile$JarFileEntry` >> compute the `certs` and `signers` field lazily. So when such `JarEntry` is >> passed to the ("copy") constructor of `JarEntry`, `null` values are captured >> for those fields. The removal of the overridden methods thus meant that the >> explicit calls to `JarEntry.getCertificates/getCodeSigners()` to compute the >> certs and signers no longer happened. This is what caused the regression. >> >> This also exposed the lack of tests in this area. I have now introduced a >> jtreg test to reproduce the issue and verify the fix. The fix reintroduces >> the overrides in `URLJarFile$URLJarFileEntry` and explicitly calls the >> `getCertificates()` and `getCodeSigners()` on the underlying `je` JarEntry. >> At the same time, it retains the original goal of JDK-8377338 and doesn't >> clone the returned arrays to prevent the duplicated array cloning. >> >> P.S: I am willing to completely backout the change done in JDK-8377338 and >> retain just this new test, if that's preferred. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Revert "Daniel's review - completely undo the changes done in JDK-8377338" > > This reverts commit 697a251ed6488e553a204f9aa16e3eb8a2a9d2f0. > - verify getCertificates() and getCodeSigners() from JarURLConnection return > new arrays on each invocation > - merge latest from master branch > - Daniel's review - completely undo the changes done in JDK-8377338 > - 8378003: JarURLConnection.getCertificates() incorrectly returns null for > signed JAR files after JDK-8377338 > - Add a test src/java.base/share/classes/sun/net/www/protocol/jar/URLJarFile.java line 202: > 200: @Override > 201: public CodeSigner[] getCodeSigners() { > 202: // super.getCodeSigners() returns CodeSigner that were s/CodeSigner/CodeSigners/ test/jdk/sun/net/www/protocol/jar/JarURLConnectionCertsAndCodeSigners.java line 183: > 181: } > 182: > 183: private static void checkSignedBy(JarEntry e, String expectedDn) > throws Exception { Not called by any code? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29748#discussion_r2823124097 PR Review Comment: https://git.openjdk.org/jdk/pull/29748#discussion_r2823161413
