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.lev...@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-99206970363698485...@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.lev...@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.redes...@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-99206970363698485...@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 <hbo...@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
