Re: RFR: 8172366: Support SHA-3 based signatures [v4]
On Tue, 15 Sep 2020 11:54:19 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update DSA.java >> >> Removed the trailing white spaces in previous update. > > The new constants in MGF1ParameterSpec.java should have "@since 16". > > > The new constants in MGF1ParameterSpec.java should have "@SInCE 16". Yes, made the change in last commit. - PR: https://git.openjdk.java.net/jdk/pull/102
Integrated: 8172366: Support SHA-3 based signatures
On Thu, 10 Sep 2020 01:58:09 GMT, Valerie Peng wrote: > Could someone please help review this RFE? > > Enhance default JDK providers except SunPKCS11 with signatures using SHA-3 > family of digests. SunPKCS11 provider will > be updated separately (JDK-8242332). > This changes covers SUN, SunRsaSign, and SunEC providers. Changes are > straightforward, just add SHA-3 digests to > various signature algorithms. > Please review the corresponding CSR as well. It's at: > https://bugs.openjdk.java.net/browse/JDK-8252260 > > Thanks! > Valerie This pull request has now been integrated. Changeset: 40206822 Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/40206822 Stats: 681 lines in 20 files changed: 10 ins; 588 del; 83 mod 8172366: Support SHA-3 based signatures Enhance default JDK providers including SUN, SunRsaSign, and SunEC, with signatures using SHA-3 family of digests. Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/102
Re: RFR: 8172366: Support SHA-3 based signatures [v5]
> Could someone please help review this RFE? > > Enhance default JDK providers except SunPKCS11 with signatures using SHA-3 > family of digests. SunPKCS11 provider will > be updated separately (JDK-8242332). > This changes covers SUN, SunRsaSign, and SunEC providers. Changes are > straightforward, just add SHA-3 digests to > various signature algorithms. > Please review the corresponding CSR as well. It's at: > https://bugs.openjdk.java.net/browse/JDK-8252260 > > Thanks! > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Update MGF1ParameterSpec.java Added "@since 16" to the new SHA-3 constants. - Changes: - all: https://git.openjdk.java.net/jdk/pull/102/files - new: https://git.openjdk.java.net/jdk/pull/102/files/62fb5a39..1274c9b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=102=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=102=03-04 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/102.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/102/head:pull/102 PR: https://git.openjdk.java.net/jdk/pull/102
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 16:59:59 GMT, Lance Andersen wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > I am fine with this as well. I will pull over the change and just sanity > check it via mach5 and then will sponsor @jaikiran Please go ahead and integrate this and I can then sponsor (has to be done in that order) - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:45:05 GMT, Jaikiran Pai wrote: > As for this: > >> As long as the input stream close() method is idem potent this should be >> safe, and AFAICS that is the case for the two >> input stream subclasses that can be returned by ZipFile::getInputStream. > > I'm curious, in the context of this change, why idempotency would be a > necessity. Would there be a "double close" > somehow on this InputStream instance? I think I understand what you meant. You were perhaps talking about the `JarFile.close` triggering the `Cleanable` to close this `InputStream` in addition to the `try-with-resources` already calling `close` on that stream. Like you rightly note, the implementation of those streams already handles that aspect correctly. - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:45:05 GMT, Jaikiran Pai wrote: > I'm curious, in the context of this change, why idempotency would be a > necessity. Would there be a "double close" > somehow on this `InputStream` instance? My bad - I hadn't realised closing the input stream would also remove it from the Cleanable resource set, so I thought it might be closed again when the jar file is closed. - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:33:51 GMT, Daniel Fuchs wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove "final" > > Hi Jaikiran, > > This is not an area I know too well - so I won't review formally, but the > proposed changes look reasonable to me. > Closing the stream from within `JarFile` after creating the manifest looks > innocuous and should release any resource > held by the stream earlier instead of waiting for the `JarFile` to be closed. > As long as the input stream `close()` > method is idem potent this should be safe, and AFAICS that is the case for > the two input stream subclasses that can be > returned by `ZipFile::getInputStream`. WRT to the claims in the JBS issue I > see that that was logged against Java 6: > there was no `Cleanable` at this time and it is possible that the internals > of ZipFile/JarFile were quite different. Thank you for the review Daniel. > WRT to the claims in the JBS issue I see that that was logged against Java 6: > there was no Cleanable at this time and > it is possible that the internals of ZipFile/JarFile were quite different. You are right. I hadn't paid attention to that detail. It's likely that it might have been behaving differently at that time. As for this: > As long as the input stream close() method is idem potent this should be > safe, and AFAICS that is the case for the two > input stream subclasses that can be returned by ZipFile::getInputStream. I'm curious, in the context of this change, why idempotency would be a necessity. Would there be a "double close" somehow on this `InputStream` instance? - PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]
On Tue, 15 Sep 2020 15:29:44 GMT, Alan Bateman wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove "final"
>
> src/java.base/share/classes/java/util/jar/JarFile.java line 428:
>
>> 426: try (final InputStream is =
>> super.getInputStream(manEntry)) {
>> 427: man = new Manifest(is, getName());
>> 428: }
>
> There is a cleaner so shouldn't have a leak, even if the JarFile is not
> explicitly closed.
> The noisy "final" can be dropped, otherwise looks good.
Thank you for the review Alan. I've updated this PR to remove the `final`. And
yes as you note, this doesn't really
leak. This change closes the InputStream earlier, as soon as it is done,
instead of waiting for the `Cleaner` to kick
in.
-
PR: https://git.openjdk.java.net/jdk/pull/186
Re: RFR: 8172366: Support SHA-3 based signatures [v4]
On Tue, 15 Sep 2020 01:34:56 GMT, Valerie Peng wrote: >> Could someone please help review this RFE? >> >> Enhance default JDK providers except SunPKCS11 with signatures using SHA-3 >> family of digests. SunPKCS11 provider will >> be updated separately (JDK-8242332). >> This changes covers SUN, SunRsaSign, and SunEC providers. Changes are >> straightforward, just add SHA-3 digests to >> various signature algorithms. >> Please review the corresponding CSR as well. It's at: >> https://bugs.openjdk.java.net/browse/JDK-8252260 >> >> Thanks! >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Update DSA.java > > Removed the trailing white spaces in previous update. The new constants in MGF1ParameterSpec.java should have "@since 16". - PR: https://git.openjdk.java.net/jdk/pull/102
