On Fri, 21 May 2021 15:39:33 GMT, Aleksei Voitylov <[email protected]>
wrote:
>> src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java line
>> 262:
>>
>>> 260: } finally {
>>> 261: releaseNativeLibraryLock(name);
>>> 262: }
>>
>> The new locking scheme looks incorrect to me. It now seems possible for 2
>> different class loaders in 2 different threads to load the same library
>> (which should not be possible).
>>
>> Thread 1:
>> - acquires name lock
>> - checks library name is already in `loadedLibraryNames` (it's not)
>> - release name lock
>> - start loading library
>>
>> Then thread 2 comes in and does the same
>>
>> Then again thread 1 finishes loading and:
>> - acquires name lock
>> - puts library name in `loadedLibraryNames`
>> - puts library name in it's own `libraries`
>> - release lock
>>
>> Then thread 2 comes in and does the same again (although adding the name to
>> `loadedLibraryNames` will silently return `false`).
>>
>> It seems that this scenario is possible, and the library will be loaded by 2
>> different class loaders, each with their own `lib` object. (If there's a
>> race, there has to be a loser as well, which would need to discard their
>> work when finding out they lost)
>>
>> You might be able to stress this by checking if
>> `loadedLibraryNames.add(name);` returns `true`, and throwing an exception if
>> not.
>>
>> I think a possible solution would be to claim the library name up front for
>> a particular loader. Then 2 threads can still race to load the same library
>> for the same class loader, but 2 threads with 2 different class loaders
>> racing to load the library should not be possible.
>>
>> Actually, you might be able to prevent a race on JNI_OnLoad altogether by
>> claiming the library name for a particular thread upfront as well (e.g.
>> using a `ConcurrentHashMap<String, Thread>`).
>
> Thank you, the previous version of the PR suffered from other problems as
> well.
>
> The PR is now reverted to a scheme with a lock held on library name, thus
> such a situation is no longer possible.
Ok, thanks.
FWIW, I think trying to lock on a per-name basis instead of globally is a good
idea, and should improve the current situation. It is also safe to do as far as
I can see.
-------------
PR: https://git.openjdk.java.net/jdk/pull/3976
