Is SharedSecrets thread-safe?
Hello, the class `jdk.internal.access.SharedSecrets` provides getter methods which all look similar to this: ``` if (static_field == null) { initialize(); } return static_field; ``` However, neither the static fields are `volatile` nor are the getter methods synchronized. So if my understanding of the Java Memory Model is correct, the compiler is free to reorder the two static field reads. So it is in theory possible that the first read yields a non-`null` value, but the second read yields a `null` value which leads to incorrect behavior (for further reading [1]). It is probably rather unlikely that this actually happens because `SharedSecrets` is in most cases accessed from static initializers (which are only run once) and because not many classes access the same `SharedSecrets` fields. Is this analysis correct or did I forget to consider parts of the Memory Model logic, or is there some JVM magic I am missing? Kind regards [1] https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-...
Depends on what `initialize()` is. If it (at least) reads a volatile field, then the compiler can't reorder the second read before the first. - Johannes On 29-Dec-20 14:42, some-java-user-99206970363698485155@vodafonemail.de wrote:
Hello, the class `jdk.internal.access.SharedSecrets` provides getter methods which all look similar to this: ``` if (static_field == null) { initialize(); } return static_field; ```
However, neither the static fields are `volatile` nor are the getter methods synchronized. So if my understanding of the Java Memory Model is correct, the compiler is free to reorder the two static field reads. So it is in theory possible that the first read yields a non-`null` value, but the second read yields a `null` value which leads to incorrect behavior (for further reading [1]).
It is probably rather unlikely that this actually happens because `SharedSecrets` is in most cases accessed from static initializers (which are only run once) and because not many classes access the same `SharedSecrets` fields.
Is this analysis correct or did I forget to consider parts of the Memory Model logic, or is there some JVM magic I am missing?
Kind regards
[1] https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-...
All the shared secrets are injected as a side effect of loading the class the getter ensures is initialized - which should provide the necessary constraints to ensure there is no way for a reordering to happen where the access object returned can be observed to be null. E.g. if (javaSecuritySignatureAccess == null) { ensureClassInitialized(Signature.class); } return javaSecuritySignatureAccess; Signature.java: static { SharedSecrets.setJavaSecuritySignatureAccess(...); } /Claes On 2020-12-29 14:55, Johannes Kuhn wrote:
Depends on what `initialize()` is.
If it (at least) reads a volatile field, then the compiler can't reorder the second read before the first.
- Johannes
On 29-Dec-20 14:42, some-java-user-99206970363698485155@vodafonemail.de wrote:
Hello, the class `jdk.internal.access.SharedSecrets` provides getter methods which all look similar to this: ``` if (static_field == null) { initialize(); } return static_field; ```
However, neither the static fields are `volatile` nor are the getter methods synchronized. So if my understanding of the Java Memory Model is correct, the compiler is free to reorder the two static field reads. So it is in theory possible that the first read yields a non-`null` value, but the second read yields a `null` value which leads to incorrect behavior (for further reading [1]).
It is probably rather unlikely that this actually happens because `SharedSecrets` is in most cases accessed from static initializers (which are only run once) and because not many classes access the same `SharedSecrets` fields.
Is this analysis correct or did I forget to consider parts of the Memory Model logic, or is there some JVM magic I am missing?
Kind regards
[1] https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-...
On Tue, Dec 29, 2020 at 5:56 AM Johannes Kuhn <info@j-kuhn.de> wrote:
Depends on what `initialize()` is.
If it (at least) reads a volatile field, then the compiler can't reorder the second read before the first.
- Johannes
I disagree with this claim. I have no idea whether concurrency is possible here, so it may not matter. See below.
On 29-Dec-20 14:42, some-java-user-99206970363698485155@vodafonemail.de wrote:
Hello, the class `jdk.internal.access.SharedSecrets` provides getter methods
which all look similar to this:
``` if (static_field == null) { initialize(); } return static_field; ```
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
It's a bit more complicated - after all we are talking about the memory model and class loading. There are some shared secrets that can be loaded later (e.g. AWT), when Threads are there. The field is only set in one single place inside the JDK, so we are talking about 3 scenarios: * The class is not yet loaded - the field is null, call ensureClassInitialized - which executes the class initializer on the current thread. A subsequent read of the field must be visible - as actions performed in the thread appear to be sequential for that thread. * The class has been successfully loaded, everything relating to that is visible. * The class is currently initialized by an other thread: * The first load of the field yields null - ensureClassInitialized blocks until the class has been loaded. * The first load yields something different than null. The question now is: can the second load yield the previous value? That is: can a field revert to it's original value? Other fields may not be visible yet (except if they have been frozen). So the big question is: can the second read return null after the first has found it to be a non-null value? After reading the "Double checked locking is broken"[1] declaration again - it says that it does work for primitive values that can't be teared. The problem seems to be that the field of the referenced objects are not yet constructed. The good news: the implementations of the shared secrets don't have fields. The better news: reference assignment also can't be teared. - Johannes [1]: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html On 29-Dec-20 18:53, Hans Boehm wrote:
On Tue, Dec 29, 2020 at 5:56 AM Johannes Kuhn <info@j-kuhn.de <mailto:info@j-kuhn.de>> wrote:
Depends on what `initialize()` is.
If it (at least) reads a volatile field, then the compiler can't reorder the second read before the first.
- Johannes
I disagree with this claim. I have no idea whether concurrency is possible here, so it may not matter. See below.
On 29-Dec-20 14:42,
some-java-user-99206970363698485155@vodafonemail.de <mailto:some-java-user-99206970363698485155@vodafonemail.de>
wrote:
Hello, the class `jdk.internal.access.SharedSecrets` provides getter methods which all look similar to this: ``` if (static_field == null) { initialize(); } return static_field; ```
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
What guarantees that if the first read does not see null, the second read will not see null? Sections 4.2 and 4.4 of Aleksey’s “Close Encounters of the JMM Kind” seem to show that such scenario is possible in face of concurrent setting of non-volatile variable. https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-hb-actu... https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-unobser... On Tue, Dec 29, 2020 at 3:17 PM Johannes Kuhn <info@j-kuhn.de> wrote:
It's a bit more complicated - after all we are talking about the memory model and class loading.
There are some shared secrets that can be loaded later (e.g. AWT), when Threads are there.
The field is only set in one single place inside the JDK, so we are talking about 3 scenarios:
* The class is not yet loaded - the field is null, call ensureClassInitialized - which executes the class initializer on the current thread. A subsequent read of the field must be visible - as actions performed in the thread appear to be sequential for that thread.
* The class has been successfully loaded, everything relating to that is visible.
* The class is currently initialized by an other thread: * The first load of the field yields null - ensureClassInitialized blocks until the class has been loaded. * The first load yields something different than null. The question now is: can the second load yield the previous value? That is: can a field revert to it's original value? Other fields may not be visible yet (except if they have been frozen). So the big question is: can the second read return null after the first has found it to be a non-null value?
After reading the "Double checked locking is broken"[1] declaration again - it says that it does work for primitive values that can't be teared. The problem seems to be that the field of the referenced objects are not yet constructed.
The good news: the implementations of the shared secrets don't have fields. The better news: reference assignment also can't be teared.
- Johannes
[1]: https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
On 29-Dec-20 18:53, Hans Boehm wrote:
On Tue, Dec 29, 2020 at 5:56 AM Johannes Kuhn <info@j-kuhn.de <mailto:info@j-kuhn.de>> wrote:
Depends on what `initialize()` is.
If it (at least) reads a volatile field, then the compiler can't
reorder
the second read before the first.
- Johannes I disagree with this claim. I have no idea whether concurrency is possible here, so it may not matter. See below.
On 29-Dec-20 14:42,
some-java-user-99206970363698485155@vodafonemail.de <mailto:some-java-user-99206970363698485155@vodafonemail.de>
wrote:
Hello, the class `jdk.internal.access.SharedSecrets` provides getter methods which all look similar to this: ``` if (static_field == null) { initialize(); } return static_field; ```
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
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@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
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@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@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
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 On Tue, Dec 29, 2020 at 4:55 PM Claes Redestad <claes.redestad@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@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@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
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@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@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@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
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... 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@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@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@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@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
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@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... 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@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@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@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@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
This brings up some stuff I wanted to mention for some time: * Console.cs is one of the fields projects like JRuby hack into (at least in the past). My guess is that they handle encodings in Ruby, and not using the Java facilities for that. The fact that it is also exported as shared secret for some unrelated stuff (sun.security.util) shows that there might be a need for a supported, public API. * java.io.Console has a nice API - but it only works (at least on Windows) if there is indeed a console attached. It usually doesn't work inside an IDE. Is there an interest to allow IDEs to create their own Console implementation, exposed through SPI? I would volunteer to write the spec and a patch, but there should be some general interest for this - as someone has to sponsor that. (And as a fair warning: It would be my first bigger contribution to OpenJDK.) - Johannes On 04-Jan-21 22:48, Peter Levart wrote:
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@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...
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@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@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@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@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
On 04/01/2021 23:09, Johannes Kuhn wrote:
This brings up some stuff I wanted to mention for some time:
* Console.cs is one of the fields projects like JRuby hack into (at least in the past). My guess is that they handle encodings in Ruby, and not using the Java facilities for that.
The fact that it is also exported as shared secret for some unrelated stuff (sun.security.util) shows that there might be a need for a supported, public API. This was discussed here at length in 2016 but there wasn't agreement to expose an API at the time. At issue is that Console is an API for reading Strings and char[], not bytes. Maybe it needs to be looked at again but we should minimally change the security password code to use the internal property so that this shared secret can be removed.
-Alan.
On 04/01/2021 23:09, Johannes Kuhn wrote:
:
* java.io.Console has a nice API - but it only works (at least on Windows) if there is indeed a console attached. It usually doesn't work inside an IDE.
Is there an interest to allow IDEs to create their own Console implementation, exposed through SPI? I would volunteer to write the spec and a patch, but there should be some general interest for this - as someone has to sponsor that. (And as a fair warning: It would be my first bigger contribution to OpenJDK.)
If you have cycles to explore, prototype, and write-up options then it could be useful for the discussion. The issue of System::console has come up once or twice in the context of IDEs. It may be that we don't need a provide implementation that is located by ServiceLoader, instead it might be that the JDK just needs something specified to the java launcher to run it with an alternative Console implementation. -Alan
Hello Peter, feel free to consider these issues one at a time, or not at all, if you don't think that it is worth it. However, please note that I will unsubscribe from this mailing list in the next days again due to the high degree of activity. (Related: https://mail.openjdk.java.net/pipermail/discuss/2020-December/005674.html) Kind regards
Peter Levart <peter.levart@gmail.com> hat am 4. Januar 2021 um 22:48 geschrieben:
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@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... 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@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@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@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@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
* some-java-user:
However, neither the static fields are `volatile` nor are the getter methods synchronized. So if my understanding of the Java Memory Model is correct, the compiler is free to reorder the two static field reads. So it is in theory possible that the first read yields a non-`null` value, but the second read yields a `null` value which leads to incorrect behavior (for further reading [1]).
[1] https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/#wishful-benign-...
Can the JVM be multi-threaded at this point? If not, program order results in the desired behavior.
participants (8)
-
Alan Bateman
-
Brett Okken
-
Claes Redestad
-
Florian Weimer
-
Hans Boehm
-
Johannes Kuhn
-
Peter Levart
-
some-java-user-99206970363698485155＠vodafonemail.de