Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
On Fri, 15 Apr 2022 18:48:33 GMT, Brent Christian wrote: >>> CleanerFactory is in java.base module, and does not export to >>> jdk.crypto.cryptoki module. I'm not sure if adding modules dependency is >>> good or not. > > It seems fine to me to export the common Cleaner to `jdk.crypto.cryptoki`. > It's already [exported for desktop and > Panama](https://github.com/bchristi-git/jdk/blob/master/src/java.base/share/classes/module-info.java#L226), > and I see that > [com.sun.crypto.provider](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/com/sun/crypto/provider/DESKey.java#L84) > already uses it. There are several places that don't use CleanerFactory here and there. I may fill a new PR later for this purpose. - PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
On Fri, 15 Apr 2022 15:00:18 GMT, Xue-Lei Andrew Fan wrote: > > CleanerFactory is in java.base module, and does not export to > > jdk.crypto.cryptoki module. I'm not sure if adding modules dependency is > > good or not. It seems fine to me to export the common Cleaner to `jdk.crypto.cryptoki`. It's already [exported for desktop and Panama](https://github.com/bchristi-git/jdk/blob/master/src/java.base/share/classes/module-info.java#L226), and I see that [com.sun.crypto.provider](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/com/sun/crypto/provider/DESKey.java#L84) already uses it. - PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
On Fri, 15 Apr 2022 08:25:40 GMT, Daniel Fuchs wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Don't use lambda in cleaner > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyStore.java > line 235: > >> 233: this.password = password.clone(); >> 234: P11Util.cleaner.register(this, >> 235: () -> Arrays.fill(this.password, ' ')); > > This lambda most probably capture `this` so it will create a leak. I was wondering to fix it in another PR as there is another issue in the block. Better to fix it sooner. - PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
On Fri, 15 Apr 2022 13:32:10 GMT, altrisi wrote: > Can't this use `CleanerFactory.cleaner()` and reuse the common Cleaner > instead of having its own? CleanerFactory is in java.base module, and does not export to jdk.crypto.cryptoki module. I'm not sure if adding modules dependency is good or not. - PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
On Fri, 15 Apr 2022 07:20:42 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Don't use lambda in cleaner
Can't this use `CleanerFactory.cleaner()` and reuse the common Cleaner instead
of having its own?
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
On Fri, 15 Apr 2022 07:20:42 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Don't use lambda in cleaner
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyStore.java line
235:
> 233: this.password = password.clone();
> 234: P11Util.cleaner.register(this,
> 235: () -> Arrays.fill(this.password, ' '));
This lambda most probably capture `this` so it will create a leak.
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v3]
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
Xue-Lei Andrew Fan has updated the pull request incrementally with one
additional commit since the last revision:
Don't use lambda in cleaner
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8248/files
- new: https://git.openjdk.java.net/jdk/pull/8248/files/d9f4e00d..cdd609c1
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8248=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8248=01-02
Stats: 14 lines in 1 file changed: 11 ins; 1 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/8248.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8248/head:pull/8248
PR: https://git.openjdk.java.net/jdk/pull/8248
