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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Tobias Hartmann-2
Hi,

please review the following patch:
https://bugs.openjdk.java.net/browse/JDK-8191360
http://cr.openjdk.java.net/~thartmann/8191360/webrev.00/

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
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Ivanov
Looks good.

Best regards,
Vladimir Ivanov

PS: I'd prefer to see library-related code factored out and the library
handle shared among 4 consecutive calls in NL::lookup_critical_entry().
But if you think it's too intrusive for the fix, please, file an RFE.

On 12/4/17 7:42 PM, Tobias Hartmann wrote:

> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8191360
> http://cr.openjdk.java.net/~thartmann/8191360/webrev.00/
>
> 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
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Tobias Hartmann-2
Hi Tobias,

On 5/12/2017 2:42 AM, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8191360
> http://cr.openjdk.java.net/~thartmann/8191360/webrev.00/

The actual fix seems fine.

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.

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

Thanks,
David

> 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
>
Reply | Threaded
Open this post in threaded view
|

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

Tobias Hartmann-2
In reply to this post by Vladimir Ivanov
Hi Vladimir,

thanks for the review!

On 04.12.2017 19:35, Vladimir Ivanov wrote:
> PS: I'd prefer to see library-related code factored out and the library handle shared among 4 consecutive calls in
> NL::lookup_critical_entry(). But if you think it's too intrusive for the fix, please, file an RFE.

Yes, I've thought about this as well but decided not to do the refactoring since it's non-trivial and the fix might be
backported.

I'll file a separate RFE for that.

Best regards,
Tobias

> On 12/4/17 7:42 PM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8191360
>> http://cr.openjdk.java.net/~thartmann/8191360/webrev.00/
>>
>> 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
>>
Reply | Threaded
Open this post in threaded view
|

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

Tobias Hartmann-2
In reply to this post by David Holmes
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
>>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Looks good!

Thanks,
David

On 5/12/2017 4:27 PM, Tobias Hartmann wrote:

> 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
>>>
Reply | Threaded
Open this post in threaded view
|

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

Tobias Hartmann-2
Thanks David.

Best regards,
Tobias

On 05.12.2017 07:30, David Holmes wrote:

> Looks good!
>
> Thanks,
> David
>
> On 5/12/2017 4:27 PM, Tobias Hartmann wrote:
>> 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
>>>>