Is SharedSecrets thread-safe?
Peter Levart
peter.levart at gmail.com
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 vodafonemail.de
wrote:
> 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.
>
> - `java.io.Console.cons`
> 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* `java.io.Console` 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 `sun.security.util.Password.convertToBytes(char[])` 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:
> https://github.com/openjdk/jdk/blob/8435f0daf2413a4c17578dd288e093fe006b3880/src/java.desktop/share/classes/sun/awt/AppContext.java#L866
> 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 gmail.com> 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:
>> https://bugs.openjdk.java.net/browse/JDK-8259021
>>
>> And prepared a PR here: https://github.com/openjdk/jdk/pull/1914
>>
>>
>> Happy new year,
>>
>> Peter
>>
>>
>>> On Tue, Dec 29, 2020 at 4:55 PM Claes Redestad <claes.redestad at oracle.com>
>>> 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 vodafonemail.de
>>>> 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("https://oracle.com");
>>>>> 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]
>>>> https://docs.oracle.com/javase/specs/jls/se15/html/jls-12.html#jls-12.4.2
>>>>>> Hans Boehm <hboehm at google.com> 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