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
