Is SharedSecrets thread-safe?

Peter Levart peter.levart at
Mon Jan 4 21:48:00 UTC 2021

Hello 99206970363698485155,

Thanks for these pointers. I would prefer to untangle those knots one at 
a time, if you don't mind, since some of them touch other parts of code 
while this change to SharedSecrets is pretty straightforward and localized.

Regards, Peter

On 12/31/20 6:25 PM, some-java-user-99206970363698485155 at 
> Hello Peter,
> the changes look good, however there might be more to consider:
> - `jdk.internal.access.SharedSecrets.getJavaLangRefAccess()`
>    Might be good to add a comment there or for `java.lang.ref.Reference` that it is (hopefully?)
>    initialized during JVM start-up. Similar to the comment for `AccessibleObject` which states
>    that it is initialized in "initPhase1".
>    Currently without special knowledge about JVM start-up, it is not obvious that this construct
>    is safe.
> - ``
>    This is pretty brittle. It is currently only thread-safe because the only accessor is
>    `System.console()` which has its own synchronization. However, I doubt that the author was aware
>    that `Console.cons` on its own is not thread-safe.
>    In general, why is there lazy initialization twice? Once in `System.console()` and then in the
>    accessor for `Console.cons`. In my opinion *only* `` should have the lazy
>    initialization logic (with proper synchronization!) and make sure that only one `Console` instance
>    exists.
> - `jdk.internal.access.JavaIOAccess.charset()`
>    The implementation of this one is extremely brittle. While it documents that the `Password` class
>    calling it will make sure that `System.console()` has been called before, in that `Password` class
>    there is not such notice, so it could easily happen that someone breaks this at a later point.
>    Additionally the field `Console.cs` it accesses is not `final`, making it even more brittle.
>    In my opinion there should be two changes:
>    1. Change `JavaIOAccess.charset()` -> `charset(Console)`, which then accesses the charset from that
>       Console argument.
>       This enforces that the user of the access already has the Console initialized and indirectly
>       establishes the happens-before relationship for `Console.cs`.
>    2. The Console class should have the following fields `final`:
>       readLock, writeLock, reader, out, pw, formatter, cs
>       For `cs` this would merely require introducing a local variable.
>    In general `[])` is problematic because it makes
>    passwords unportable when one OS uses a different terminal encoding than another.
>    The cleaner backward compatible solution might be to simply block all non-ASCII chars (i.e. throw
>    exception for them)?
>    This would break for all existing users which used non-ASCII chars or where their terminal has an
>    encoding not based on ASCII, but these users might already have different problems due to the
>    existing implementation.
> - `jdk.internal.access.SharedSecrets.setJavaAWTAccess(JavaAWTAccess)`
>    Might have to establish a happens-before relationship for `getJavaAWTAccess()` because caller
>    appears to expect a `JavaAWTAccess` in case respective class has been loaded.
>    On a side note here: The implementation of that access appears to contain redundant code:
>    The null check there makes no sense because `ecx` is always null at that point.
> - `jdk.internal.access.SharedSecrets.setJavaAWTFontAccess(JavaAWTFontAccess)`
>    This seems to be rather brittle as well because its callers check whether the access has already
>    been initialized.
>    Additionally this seems to be more complicated than it has to be: It appears to be simpler to have
>    `SharedSecrets` initialize the access by initializing only `NumericShaper` (or `TextAttribute`,
>    ultimately does not matter which class from that package is used) when `getJavaAWTFontAccess()` is
>    called. The performance penalty (if any) is likely negligible.
> - `com.sun.jmx.mbeanserver.JavaBeansAccessor`
>    Since the static initializer of that class is loading `Introspector` (which sets access object)
>    anyways it might be saner to have this initialization logic in `SharedSecrets` instead.
> Kind regards
>> Peter Levart <peter.levart at> hat am 31. Dezember 2020 um 11:07 geschrieben:
>> On 12/31/20 2:30 AM, Hans Boehm wrote:
>>> It sounds as though this would be correct if
>>> if (static_field == null) {
>>>     initialize();
>>> }
>>> return static_field;
>>> ```
>>> were rewritten as
>>> Foo my_local_copy = static_field;
>>> if (my_copy == null) {
>>>      initialize();
>>>      my_local_copy = static_field;
>>> }
>>> return my_local_copy;
>>> That would prevent the redundant read of static_field unless this thread
>>> did the initialization, in which case the original null would no longer be
>>> visible to the second read.
>>> Hans
>> I agree. The initialize() part is triggering some class initialization
>> where concurrent attempts do establish a HB edge so the 2nd read of
>> static_field after initialize() is ordered properly and can't read null.
>> I created a JIRA ticket here:
>> And prepared a PR here:
>> Happy new year,
>> Peter
>>> On Tue, Dec 29, 2020 at 4:55 PM Claes Redestad <claes.redestad at>
>>> wrote:
>>>> Hans' assessment seems about right in the generic case he's describing.
>>>> But consider:
>>>> 1. There is no concurrent setting of anything here - it's done in a
>>>> static initializer which will happen exactly once by the thread
>>>> initializing the class ($ 12.2.4 item 9).
>>>> 2. While there is a data race on the access of the fields in
>>>> SharedSecrets, all of the Access instances are stateless. This means the
>>>> race is benign in the sense that when reading the field only a null or
>>>> a safely published instance can be observed.
>>>> I wouldn't swear there's a strict guarantee a null can never be returned
>>>> from a SharedSecrets accessor though. Perhaps that's something that
>>>> could be hardened.
>>>> /Claes
>>>> On 2020-12-30 00:32, some-java-user-99206970363698485155 at
>>>> wrote:
>>>>> That would also be my understanding of the current situation, though
>>>> this contradicts what
>>>>> Claes wrote.
>>>>> Maybe the JVM behaves in a way which does not allow reordering, but the
>>>> JLS definitely seems
>>>>> to allow it. Section § 12.2.4 [0] only mentions that for the class to be
>>>> initialized there
>>>>> has to exist a lock LC (or at least the happens-before relationship),
>>>> but there is no
>>>>> "freeze the world" or anything similar which would force a
>>>> happens-before relationship
>>>>> for the code in `SharedSecrets`.
>>>>> Maybe most of the `SharedSecrets` methods are thread-safe (albeit
>>>> extremely brittle) because
>>>>> the classes to which the accessor objects belong to have previously
>>>> already been loaded
>>>>> before `SharedSecrets` is used, therefore having already established a
>>>> happens-before
>>>>> relationship.
>>>>> However, this is definitely not the case for all of the methods as shown
>>>> by the following
>>>>> example:
>>>>> ```
>>>>> CookieHandler.setDefault(new CookieHandler() {
>>>>>        @Override
>>>>>        public void put(URI uri, Map<String, List<String>> responseHeaders)
>>>> throws IOException { }
>>>>>        @Override
>>>>>        public Map<String, List<String>> get(URI uri, Map<String,
>>>> List<String>> requestHeaders) throws IOException {
>>>>>            return Collections.emptyMap();
>>>>>        }
>>>>> });
>>>>> // Any site which uses cookies (i.e. Set-Cookie or Set-Cookie2 header)
>>>>> URL url = new URL("");
>>>>> url.openConnection().getHeaderFields();
>>>>> ```
>>>>> Running this code with `openjdk 15 2020-09-15` shows that the call to
>>>>> `SharedSecrets.getJavaNetHttpCookieAccess()` is made before the class
>>>> `HttpCookie` has
>>>>> been accessed and initialized. Therefore merely running this code in two
>>>> separate threads
>>>>> (both having been started before the code is executed, since
>>>> `Thread.start()` establishes
>>>>> a happens-before relationship) should be enough to render that
>>>> `SharedSecrets` method
>>>>> non-thread-safe.
>>>>> Kind regards
>>>>> [0]
>>>>>> Hans Boehm <hboehm at> hat am 29. Dezember 2020 um 18:53
>>>> geschrieben:
>>>>>> If static_field is not volatile, and set concurrently, then the first
>>>> read of static_field may return non-null and the second null, without
>>>> initialize() even being executed. The Java memory model does not prevent
>>>> reordering of non-volatile reads from the same field (for good reason).
>>>>>> Even if initialize() is executed and performs a volatile read, this
>>>> reasoning doesn't hold. The initial static_field read may be delayed past
>>>> the volatile read inside the conditional and hence, at least theoretically,
>>>> past the second read. Control dependencies don't order reads, either in
>>>> Java, or in modern weakly-ordered architectures with branch prediction.
>>>> This doesn't matter if initialize() sets static_field.
>>>>>> This all assumes that having two threads call initialize() is OK.
>>>>>> Java code with data races is extremely tricky and rarely correct.
>>>>>> Hans

More information about the core-libs-dev mailing list