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

[Yasumasa Suenaga]

Hi David,

I thought "Exactly one call to a start-up function is made per agent." means "per -agentlib / -agentpath". Actually we can see following behavior:


source code (README.md is in Japanese, sorry):
   https://github.com/YaSuenag/jvmti-examples/tree/master/helloworld


Attach same agent 4 times in arguments of java:

$ java -agentpath:`pwd`/libhelloworld.so -agentpath:libhelloworld.so -agentlib:helloworld -agentlib:helloworld --version
Hello World from Agent_OnLoad()
Hello World from Agent_OnLoad()
Hello World from Agent_OnLoad()
Hello World from Agent_OnLoad()
openjdk 14.0.1 2020-04-14
OpenJDK Runtime Environment (build 14.0.1+7-Ubuntu-1ubuntu1)
OpenJDK 64-Bit Server VM (build 14.0.1+7-Ubuntu-1ubuntu1, mixed mode, sharing)
Goodbye World from Agent_OnUnload()
Goodbye World from Agent_OnUnload()
Goodbye World from Agent_OnUnload()
Goodbye World from Agent_OnUnload()

However -javaagent is handled in libinstrument.so (instrument.dll) in HotSpot, so I concern if the spec would not allow to attach same agent multiple times, `-javaagent:agentA.jar -javaagent:agentB.jar` would not work.


I understood that we should call Agent_OnUnload() should be called if Agent_OnAttach() is failed, but I want to discuss about the behavior before creating new webrev.
Currently the agent library is unloaded if Agent_OnAttach is not found from it.


Thanks,

Yasumasa


On 2020/09/02 15:06, David Holmes wrote:
> On 2/09/2020 3:47 pm, Yasumasa Suenaga wrote:
>> Hi David,
>>
>> On 2020/09/02 14:00, David Holmes wrote:
>>> Hi Yasumasa,
>>>
>>> On 1/09/2020 11:42 pm, Yasumasa Suenaga wrote:
>>>> Hi all,
>>>>
>>>> Please review this change:
>>>>
>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8252657
>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8252657/webrev.00/
>>>>
>>>> I tried to attach JVMTI agent which returns JNI_ERR in Agent_OnUnload via JVMTI.agent_load dcmd. I expected the library would be unloaded, but I saw it in VM.dynlibs dcmd even though it was failed to load.
>>>
>>> I'm not so sure there is a bug here. The specification is quite lax:
>>>
>>> "Instead the VM ignores the error, or takes some implementation specific action -- for example it might print an error to standard error, or record the error in a system log. "
>>
>> I think this spec says about VM behavior only. For example, if Agent_OnAttach is failed, the action in VM is not specified.
>>
>>
>>> So not unloading the library is not a bug in respect of the specification. Further by unloading the library do you not now permit it to be re-loaded and the OnAttach function to be executed repeatedly? That seems wrong to me. It could also be argued that if you will re-execute onAttach then you should first have called OnUnload, which you don't do.
>>
>> Administrator can retry to attach agent with another options if it was failed.
>> Currently we can attach same agent to one JVM as many times as we like. Of course Agent_OnAttach would be called in each attach operation.
> 
> The spec states for startup:
> 
> "Exactly one call to a start-up function is made per agent."
> 
> and in regards to OnUnload:
> 
> " ... and this function is also called if the library is unloaded for other reasons."
> 
> It says nothing about OnUnload requiring a successful OnAttach for it to be called.
> 
> If we can attach an agent multiple times and have OnAttach called multiple times then that seems a clear violation of the specification. At best there would have to be an intervening unloading of the agent with a call to OnUnload.
> 
>> JVMTI agent might have entry point (e.g. DllMain in Windows) and it might work something (e.g. multiple launch check). In addition it is strange agent library exists even if AgentLibrary is discarded when Agent_OnAttach is failed - nobody would not manage it!
> 
> I agree it seems cleaner to unload the library. But I'm not at all clear what the expected semantics are and exactly what complies with the specification.
> 
> David
> -----
> 
>>
>>
>> Cheers,
>>
>> Yasumasa
>>
>>
>>> So while this current behaviour may not meet with your expectations it is not a bug per-se, and the change in behaviour could have other implications that are not insignificant.
>>>
>>> If the new behaviour is considered desirable then a CSR request should also be filed for this.
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>> Please see JBS how to reproduce.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa