[10] RFR(S): 8191360: Lookup of critical JNI method causes duplicate library loading with leaking handler

[Tobias Hartmann]

Hi David,

thanks for the review!

On 04.12.2017 22:46, David Holmes wrote:
> I'm unclear why you added the critical entry to test/jdk/java/lang/ClassLoader/nativeLibrary/libnativeLibraryTest.c ??
> The impact of that on the test is not clear to me. It also obscures whether the test is fixed just by the actual fix.

Right, I should have mentioned that in the RFR. The reason is that I first had a fix that only unloaded the shared
library if no critical method was found, i.e., if critical_entry == NULL. This fixes the crash in the original test
(because there is no critical version of the method). For additional coverage, I've added a critical entry to the test
library.

Thinking about it again, the test changes are not necessary. Here's a webrev without:
http://cr.openjdk.java.net/~thartmann/8191360/webrev.01/

> I agree with Vladimir that there is scope for being more efficient with the library handling in this code, but that's
> future work.

Right, I'll file a separate RFE and link it to the bug.

Best regards,
Tobias
>> When creating a native wrapper, the compiler checks the shared library for a critical version of the native method
>> (-XX:+CriticalJNINatives, see comments in JDK-7013347 [1]) that can be called with reduced overhead. This is done in
>> NativeLookup::lookup_critical_style() with a call to dll_load() to get a handle to the (already loaded) library and a
>> subsequent call to dll_lookup() to get the entry address of the critical version.
>>
>> The problem is that without a call to dll_unload(), this handle to the library will stay live and internal reference
>> counting will prevent the library from ever being unloaded (even if the native method holder is unloaded). As a result,
>> static fields in the library code are not reset, causing the NativeLibraryTest to fail due to unexpected field values
>> once the library is loaded again.
>>
>> I've fixed this by closing the handle immediately after retrieving the critical entry address. This is fine because the
>> library is still kept alive by JNI (see JVM_LoadLibrary). As soon as the holder class and the library are unloaded (see
>> JVM_UnloadLibrary), the native wrapper that calls 'critical_entry' becomes unreachable and is unloaded as well.
>>
>> I've also thought about passing 'RTLD_NOLOAD' to dlopen [2] but although that avoids loading the library, it still
>> creates a handle (that needs to be closed) if the library is already loaded.
>>
>> Tested with failing test and Mach 5 hs-tier1, hs-tier2, hs-precheckin-comp (running).
>>
>> Thanks,
>> Tobias
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-7013347
>> [2] https://linux.die.net/man/3/dlopen
>>