On Wed, 19 May 2021 16:29:33 GMT, Aleksei Voitylov <[email protected]>
wrote:
>> Please review this PR which fixes the deadlock in ClassLoader between the
>> two lock objects - a lock object associated with the class being loaded, and
>> the ClassLoader.loadedLibraryNames hash map, locked during the native
>> library load operation.
>>
>> Problem being fixed:
>>
>> The initial reproducer demonstrated a deadlock between the JarFile/ZipFile
>> and the hash map. That deadlock exists even when the ZipFile/JarFile lock is
>> removed because there's another lock object in the class loader, associated
>> with the name of the class being loaded. Such objects are stored in
>> ClassLoader.parallelLockMap. The deadlock occurs when JNI_OnLoad() loads
>> exactly the same class, whose signature is being verified in another thread.
>>
>> Proposed fix:
>>
>> The proposed patch suggests to get rid of locking loadedLibraryNames hash
>> map and synchronize on each entry name, as it's done with class names in see
>> ClassLoader.getClassLoadingLock(name) method.
>>
>> The patch introduces nativeLibraryLockMap which holds the lock objects for
>> each library name, and the getNativeLibraryLock() private method is used to
>> lazily initialize the corresponding lock object. nativeLibraryContext was
>> changed to ThreadLocal, so that in any concurrent thread it would have a
>> NativeLibrary object on top of the stack, that's being currently
>> loaded/unloaded in that thread. nativeLibraryLockMap accumulates the names
>> of all native libraries loaded - in line with class loading code, it is not
>> explicitly cleared.
>>
>> Testing: jtreg and jck testing with no regressions. A new regression test
>> was developed.
>
> Aleksei Voitylov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix trailing whitespace
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>`).
-------------
PR: https://git.openjdk.java.net/jdk/pull/3976
