On Thu, 27 Aug 2026 05:23:19 GMT, Valerie Peng <[email protected]> wrote:
>> This PR removes the finalize() methods from the SASL modules and replace >> them with Cleaner. Also did some minor refactoring so the sensitive info is >> only kept wherever necessary. >> >> I am still working on a regression test for the GssKrb5 classes, just want >> to get the source changes out while I work on it so there is more time for >> review. >> >> Thanks in advance for the review~ >> >> --------- >> - [X] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > removed the post-disposal check as the Cleanable.clean() guarantees the > action runs at most once. Looking good. I have some suggestions. I've not yet looked at the test. src/java.base/share/classes/sun/security/util/KeyUtil.java line 559: > 557: > 558: // methods for generating cleanables which clears arrays > 559: public static Cleaner.Cleanable getCleanable(Object obj, byte[] b) { I think `registerCleaner` is a better name for this method (as is used in `GssKrb5Base`). src/java.base/share/classes/sun/security/util/KeyUtil.java line 563: > 561: ()->{ > 562: Arrays.fill(b, (byte) 0); > 563: }); This lambda looks correct for use as a cleaning action. It captures (and will maintain a reference to) `b`, as necessary, and it does not capture `obj`. src/java.security.sasl/share/classes/com/sun/security/sasl/ClientFactoryImpl.java line 146: > 144: > 145: if (pw != null) { > 146: bytepw = PBEUtil.encodePassword(pw); I see that the comment for `PBEUtil.encodePassword()` lists places it is used, if you want to consider adding `ClientFactoryImpl`, `CramMD5Server` to that list. src/java.security.sasl/share/classes/com/sun/security/sasl/CramMD5Base.java line 145: > 143: /* digest the key if longer than 64 bytes */ > 144: key = (key.length > MD5_BLOCKSIZE ? > 145: md5.digest(key) : key.clone()); What is the benefit of cloning `key`? src/java.security.sasl/share/classes/com/sun/security/sasl/CramMD5Client.java line 96: > 94: * the server. > 95: * @throws SaslException if platform does not have MD5 support > 96: * @throws IllegalStateException if this method is invoked more than > once. Perhaps update `@throws IllegalStateException` to append something like, "or if `dispose()` has been called." src/java.security.sasl/share/classes/com/sun/security/sasl/CramMD5Server.java line 254: > 252: > 253: @Override > 254: public void dispose() throws SaslException { Good to remove GC-triggered cleanup from `CramMD5Server` altogether. 🎉 src/java.security.sasl/share/classes/com/sun/security/sasl/PlainClient.java line 102: > 100: * @param challengeData Ignored > 101: * @return A non-null byte array containing the response to be sent > to the server. > 102: * @throws IllegalStateException if authentication already completed Same as `CramMD5Client`: perhaps update @throws IllegalStateException to append something like, "or if dispose() has been called." src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java line 66: > 64: throw new RuntimeException > 65: ("Problem disposing GSS context", e); > 66: }}); This lambda looks correct for use as a cleaning action. It captures (and will maintain a reference to) `ctx`, as necessary, and it does not capture `obj`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java line 82: > 80: } > 81: > 82: protected void updateSecCtx(GSSContext secCtx) throws SaslException { I think something like, `setSecCtx()` would be a better name. `secCtx` can only be "set" once, and cannot be set and then "updated" to another value. It would also be worth a comment that it can only be called once. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java line 115: > 113: if (t.name().toLowerCase(Locale.US).equals(type)) { > 114: try { > 115: return > ((ExtendedGSSContext)secCtx).inquireSecContext(t); `getNegotiatedProperty()` accesses `secCtx`, and so I recommend the method code be enclosed by a `try/finally/reachabilityFence()`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java line 157: > 155: } catch (GSSException e) { > 156: throw new SaslException("Problems unwrapping SASL buffer", > e); > 157: } I recommend adding `finally/reachabilityFence()`, as `unwrap()` accesses `secCtx`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Base.java line 184: > 182: } catch (GSSException e) { > 183: throw new SaslException("Problem performing GSS wrap", e); > 184: } I recommend adding `finally/reachabilityFence()`, as `wrap()` accesses `secCtx`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Client.java line 171: > 169: } catch (GSSException e) { > 170: throw new SaslException("Failure to initialize security > context", e); > 171: } I recommend adding `finally/reachabilityFence()`, as `secCtx` is accessed after registering with Cleaner on L124. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Client.java line 231: > 229: } catch (GSSException e) { > 230: throw new SaslException("GSS initiate failed", e); > 231: } I recommend adding `finally/reachabilityFence()`, as `evaluateChallenge()` accesses `secCtx`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Client.java line 352: > 350: } catch (GSSException e) { > 351: throw new SaslException("Final handshake failed", e); > 352: } I recommend adding `finally/reachabilityFence()`, as `doFinalHandshake()` accesses `secCtx`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Server.java line 128: > 126: } catch (GSSException e) { > 127: throw new SaslException("Failure to initialize security > context", e); > 128: } I recommend adding `finally/reachabilityFence()`, as `secCtx` is accessed after registering with Cleaner on L115. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Server.java line 203: > 201: } catch (GSSException e) { > 202: throw new SaslException("GSS initiate failed", e); > 203: } I recommend adding `finally/reachabilityFence()`, as `evaluateResponse()` accesses `secCtx`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Server.java line 249: > 247: } catch (GSSException e) { > 248: throw new SaslException("Problem wrapping handshake1", e); > 249: } I recommend adding `finally/reachabilityFence()`, as `doHandshake1()` accesses `secCtx`. src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Server.java line 333: > 331: } catch (IOException | UnsupportedCallbackException e) { > 332: throw new SaslException("Problem with callback handler", e); > 333: } I recommend adding `finally/reachabilityFence()`, as `doHandshake2()` accesses `secCtx`. ------------- Changes requested by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/32551#pullrequestreview-5055422998 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884337985 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884349000 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884365997 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884370230 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884396097 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884525239 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884531941 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884555889 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884565378 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884586450 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884592173 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884600580 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884623578 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884629036 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884635336 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884644799 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884652660 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884657709 PR Review Comment: https://git.openjdk.org/jdk/pull/32551#discussion_r3884661218
