RFR: 8252657: JVMTI agent is not unloaded when Agent_OnAttach is failed

David Holmes david.holmes at oracle.com
Fri Dec 18 04:00:27 UTC 2020 

On 16/12/2020 5:36 pm, Yasumasa Suenaga wrote:
> Hi David,
> 
>> I'm not sure you mean exactly by DLL unloading mechanism is not hooked.
> 
> I want to say we do not hook `FreeLibrary()` on Windows and `dlclose()` 
> on Linux.
> Can we describe ""this function may be called - " at here?

I think it is clear this refers to the VM doing the unloading not 
somebody issuing direct dlclose/FreeLibrary commands.

David

> 
> Cheers,
> Yasumasa
> 
> 
> On 2020/12/16 15:48, David Holmes wrote:
>> On 16/12/2020 4:33 pm, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>>> "This function will be called by the VM when the library is about to 
>>>> be unloaded. The library will be unloaded (unless it is statically 
>>>> linked into the executable) and this function will be called if some 
>>>> platform specific mechanism causes the unload (an unload mechanism 
>>>> is not specified in this document) or the library is (in effect) 
>>>> unloaded by the termination of the VM. VM termination includes 
>>>> normal termination and VM failure, including start-up failure, but 
>>>> not, of course, uncontrolled shutdown. An implementation may also 
>>>> choose to not call this function if the 
>>>> Agent_OnAttach/Agent_OnAttach_L function reported an error (returned 
>>>> a non-zero value)."
>>>
>>> Thank you so much!
>>> According to this, we can fix this bug simply. (We can unload agent 
>>> library without `Agent_OnUnload()` call)
>>
>> Yes I think it is reasonable to allow this. But of course there must 
>> be a general consensus.
>>
>>> However I think following sentence should be removed because DLL 
>>> unloading mechanism is not hooked, and also `Agent_OnUnload()` 
>>> wouldn't be called by dll unloading - it depends on VM termination / 
>>> failure / non-zero value from entry points.
>>>
>>> "this function will be called if some platform specific mechanism 
>>> causes the unload (an unload mechanism is not specified in this 
>>> document)"
>>
>> I'm not sure you mean exactly by DLL unloading mechanism is not 
>> hooked. But that sentence has to remain because it makes it clear that 
>> the decision as to when unloading occurs is up to an implementation 
>> and is not defined by the specification.
>>
>> Cheers,
>> David
>> -----
>>
>>>
>>> If you are OK, I will update CSR.
>>>
>>>
>>> Cheers,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/12/16 11:09, David Holmes wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Sorry for the delay getting back to this.
>>>>
>>>> On 1/12/2020 4:41 pm, Yasumasa Suenaga wrote:
>>>>> Hi David,
>>>>>
>>>>> On 2020/12/01 14:59, David Holmes wrote:
>>>>>> Looking at the original webrev from September:
>>>>>>
>>>>>> https://cr.openjdk.java.net/~ysuenaga/JDK-8252657/webrev.00/src/hotspot/share/prims/jvmtiExport.cpp.cdiff.html 
>>>>>>
>>>>>>
>>>>>> The suggestion to unload the library if Agent_OnAttach fails seems 
>>>>>> quite reasonable - it is what happens if no Agent_OnAttach 
>>>>>> function can be found in the agent.
>>>>>>
>>>>>> But the specification is lacking because it simply states any such 
>>>>>> error is "ignored" - which is an oversimplification and I think 
>>>>>> was really intended to contrast with the abort behaviour if 
>>>>>> Agent_OnLoad fails. In addition the specification indicates that 
>>>>>> Agent_OnUnload is called any time the agent library is unloaded, 
>>>>>> except for "uncontrolled shutdown". This unfortunately suggests 
>>>>>> that before unloading the library after OnAttach fails, we also 
>>>>>> call OnUnload. That seems wrong and in fact the VM does not do 
>>>>>> that - and noone seems to have complained.
>>>>>>
>>>>>> Also note the specification states an agent "must export a 
>>>>>> start-up function ..." but doesn't say what happens if it doesn't 
>>>>>> do so.
>>>>>>
>>>>>> My gut feeling for what the specification should say here is that 
>>>>>> if the start-up function does not exist, or the call to 
>>>>>> Agent_OnAttach reports an error, then the agent library is 
>>>>>> immediately unloaded with no attempt to call the agent shutdown 
>>>>>> function (Agent_OnUnload).
>>>>>
>>>>> That's what I wanted to suggest!
>>>>> I understand we need to consider following case in this issue, is 
>>>>> it right?
>>>>>
>>>>> Agent_OnLoad / Agent_OnLoad_L does not exist:
>>>>>      - Agent_OnUnload is not called
>>>>>      - DLL is not unloaded (JVM will abort)
>>>>>
>>>>> Agent_OnLoad / Agent_OnLoad_L failed:
>>>>>      - Agent_OnUnload is not called
>>>>>      - DLL is not unloaded (JVM will abort)
>>>>
>>>> These cases are already well covered.
>>>>
>>>>> Agent_OnAttach does not exist:
>>>>>      - Agent_OnUnload is not called
>>>>>      - DLL is unloaded
>>>>>
>>>>> Agent_OnAttach_L does not exist:
>>>>>      - Agent_OnUnload is not called
>>>>>      - DLL is not unloaded (static link)
>>>>
>>>> These cases are kind of implicitly covered, but could be clarified.
>>>>
>>>>> Agent_OnAttach failed:
>>>>>      - Agent_OnUnload is not called
>>>>>      - DLL is unloaded
>>>>>
>>>>> Agent_OnAttach_L faled:
>>>>>      - Agent_OnUnload is not called
>>>>>      - DLL is not unloaded (static link)
>>>>
>>>> These are the problematic cases.
>>>>
>>>>>
>>>>>> But with my "compatibility glasses" on this may be too strong to 
>>>>>> retrofit to the specification as other VMs may behave differently. 
>>>>>> So as I stated in the CSR request we probably want to allow the 
>>>>>> current hotspot behaviour but not mandate it (unless we check with 
>>>>>> all the other VM implementations that such a specification is okay 
>>>>>> with them).
>>>>>
>>>>> Ok, for example, can we change the spec as following?
>>>>> (Of course, this is not all)
>>>>>
>>>>> ```
>>>>> diff --git a/src/hotspot/share/prims/jvmti.xml 
>>>>> b/src/hotspot/share/prims/jvmti.xml
>>>>> index 44553b8065f..57c87b1a71b 100644
>>>>> --- a/src/hotspot/share/prims/jvmti.xml
>>>>> +++ b/src/hotspot/share/prims/jvmti.xml
>>>>> @@ -688,7 +688,8 @@ Agent_OnUnload_L(JavaVM *vm)</example>
>>>>>       mechanism causes the unload (an unload mechanism is not 
>>>>> specified in this document)
>>>>>       or the library is (in effect) unloaded by the termination of 
>>>>> the VM whether through
>>>>>       normal termination or VM failure, including start-up failure.
>>>>> -    Uncontrolled shutdown is, of course, an exception to this rule.
>>>>> +    Uncontrolled shutdown is, of course, an exception to this 
>>>>> rule, and also it might not be called
>>>>> +    when start-up function does not exist or fails (returns 
>>>>> non-zero value).
>>>>
>>>> I think this is what was originally proposed and as I said then I 
>>>> don't like burying this detail inside the reference to uncontrolled 
>>>> shutdown. We should make this much more explicit which requires some 
>>>> reworking of the existing far-too-long sentence.
>>>>
>>>> "This function will be called by the VM when the library is about to 
>>>> be unloaded. The library will be unloaded (unless it is statically 
>>>> linked into the executable) and this function will be called if some 
>>>> platform specific mechanism causes the unload (an unload mechanism 
>>>> is not specified in this document) or the library is (in effect) 
>>>> unloaded by the termination of the VM. VM termination includes 
>>>> normal termination and VM failure, including start-up failure, but 
>>>> not, of course, uncontrolled shutdown. An implementation may also 
>>>> choose to not call this function if the 
>>>> Agent_OnAttach/Agent_OnAttach_L function reported an error (returned 
>>>> a non-zero value)."
>>>>
>>>> Cheers,
>>>> David
>>>> -----
>>>>
>>>>>       Note the distinction between this function and the
>>>>>       <eventlink id="VMDeath">VM Death event</eventlink>: for the 
>>>>> VM Death event
>>>>>       to be sent, the VM must have run at least to the point of 
>>>>> initialization and a valid
>>>>> ```
>>>>>
>>>>>> I agree that the more general issue of re-loading an agent is a 
>>>>>> separate issue.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>> On 1/12/2020 3:21 pm, David Holmes wrote:
>>>>>>> On 1/12/2020 3:19 pm, David Holmes wrote:
>>>>>>>> On 1/12/2020 2:46 pm, Yasumasa Suenaga wrote:
>>>>>>>>> Hi Chris, David,
>>>>>>>>>
>>>>>>>>> Currently Agent_OnUnload() is not called when Agent_OnLoad() is 
>>>>>>>>> failed - JVM will abort.
>>>>>>>>> Should we re-think this behavior?
>>>>>>>>
>>>>>>>> We should we rethink that? It is probably one of the clearest 
>>>>>>>> parts of the spec. If Agent_Onload fails that is considered a 
>>>>>>>> fatal error - end of story.
>>>>>>>
>>>>>>> I meant, of course, "Why should we rethink that?".
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>> The issue is with Agent_onAttach and how its failure should, or 
>>>>>>>> should not, impact Agent_OnUnload.
>>>>>>>>
>>>>>>>> David
>>>>>>>> -----
>>>>>>>>
>>>>>>>>> https://github.com/YaSuenag/jvmti-examples/tree/master/helloworld
>>>>>>>>>
>>>>>>>>> ```
>>>>>>>>> $ java -agentpath:/path/to/libhelloworld.so=error --version
>>>>>>>>> Hello World from Agent_OnLoad()
>>>>>>>>>    options = error
>>>>>>>>> Error occurred during initialization of VM
>>>>>>>>> agent library failed to init: /path/to/libhelloworld.so
>>>>>>>>> ```
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020/12/01 11:44, David Holmes wrote:
>>>>>>>>>> On 1/12/2020 11:45 am, Chris Plummer wrote:
>>>>>>>>>>> On Fri, 2 Oct 2020 07:27:43 GMT, Yasumasa Suenaga 
>>>>>>>>>>> <ysuenaga at openjdk.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>>> * Q3: What has to be done for statically linked agent?
>>>>>>>>>>>>
>>>>>>>>>>>> JVMTI spec says "unless it is statically linked into the 
>>>>>>>>>>>> executable", so I think we can ignore about 
>>>>>>>>>>>> Agent_OnUnload_L() in this PR.
>>>>>>>>>>>
>>>>>>>>>>> I don't think that makes sense. If you call it for 
>>>>>>>>>>> dynamically linked then you need to call it for statically 
>>>>>>>>>>> linked also.
>>>>>>>>>>
>>>>>>>>>> Agreed. Even though you can't physically unload the statically 
>>>>>>>>>> linked library, if it is logically unloaded by some mechanism, 
>>>>>>>>>> then Agent_OnUnload_L is supposed to be called.
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>> -----
>>>>>>>>>>
>>>>>>>>>>> -------------
>>>>>>>>>>>
>>>>>>>>>>> PR: https://git.openjdk.java.net/jdk/pull/19
>>>>>>>>>>>

More information about the hotspot-dev mailing list