RFR: 8266310: deadlock while loading the JNI code [v3]

[Jorn Vernee]

On Fri, 21 May 2021 15:39:33 GMT, Aleksei Voitylov <avoitylov at openjdk.org> 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