On Thu, 8 Oct 2026 12:35:13 GMT, Jaikiran Pai <[email protected]> wrote:
>> Chris Hegarty 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 four additional >> commits since the last revision: >> >> - Merge remote-tracking branch 'upstream/master' into >> x500principal-safe-publication >> - volatile >> - revert >> - Make X500Principal.thisX500Name final to fix unsafe publication > > src/java.base/share/classes/javax/security/auth/x500/X500Principal.java line > 553: > >> 551: private static final Unsafe UNSAFE = Unsafe.getUnsafe(); >> 552: private static final long X500NAME_OFFSET = >> 553: UNSAFE.objectFieldOffset(X500Principal.class, "thisX500Name"); > > Hello Chris, as far as I can see, the pre-existing implementation in the > `X500Principal` class is doing the right thing by not marking the field as a > `final`, since that field does get updated after construction of the > `X500Principal`. So although this proposed solution would fix the issue, I > think it feels a bit odd to see the use of `Unsafe` and the "forced" `final` > field in this class. > > The actual issue (as you have explained in the JBS description) arises from > the implementation in `X500Name` which allows concurrent access to the > `x500Principal` field without proper concurrency protections. I think it > would be easier to follow and more natural to do the fix in `X500Name` by > introducing the necessary synchronization construct for the `x500Principal` > field. Thanks @jaikiran, that's a fair point. I agree the race comes from `X500Name.asX500Principal()` lazily setting a plain field. Another thread could see the new `X500Principal` before its `thisX500Name` was visible, and that's what causes the NPE. I've updated the PR to fix it in `X500Name` instead. `x500Principal` is now volatile, and `asX500Principal()` reads it once into a local variable. The volatile write guarantees any thread that sees the principal also sees its fully set-up state. Two threads may still each create a principal, but they're equivalent, so that's harmless. It's the same approach `X500Name` already uses for `rdnLis`t and `allAvaLis`t. I went with volatile rather than synchronized to avoid taking a lock on this path. The `X500Principa`l changes are reverted, so the PR now only touches `X500Name.java`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/33253#discussion_r4336033372
