RFR: 8307478: Implementation of Prepare to Restrict The Dynamic Loading of Agents [v7]

[Chris Plummer]

On Thu, 1 Jun 2023 06:46:41 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>>> The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. Isn't the library always already loaded by the time we get here (the assert below seems to imply that)? 
>> 
>> JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and adds it to the agent list if is succeeds. The test that you are looking is checking the list of already loaded agents. Maybe the function name "is_loaded" is the confusion here, maybe a better name is needed.
>
> I see the source of my confusion.
> The function `invoke_Agent_OnAttach` is called from the `JvmtiAgent::load`.
> But at the time of `invoke_Agent_OnAttach` the agent has not been added to the list yet.
> It is added after the `JvmtiAgent::load` with the function `JvmtiAgentList::add`:
> 
> jint JvmtiAgentList::load_agent(const char* agent_name, const char* absParam,
>                            const char* options, outputStream* st) {
>   // The abs parameter should be "true" or "false"
>   const bool is_absolute_path = (absParam != nullptr) && (strcmp(absParam, "true") == 0);
>   JvmtiAgent* const agent = new JvmtiAgent(agent_name, options, is_absolute_path, /* dynamic agent */ true);
>   if (agent->load(st)) {
>     add(agent);
>   . . .
> 
> Now I see the code is correct.
> I'd suggest to add a small comment before the line 512 saying
> that the agent has not been included into the agents list yet.

> JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and adds it to the agent list if is succeeds. The test that you are looking is checking the list of already loaded agents. Maybe the function name "is_loaded" is the confusion here, maybe a better name is needed.

Yes, I did notice that there were two different `is_loaded` checks going on. What's not clear is the relation between agent->is_loaded() and JvmtiAgentList::is_loaded(library). I assume that at this point in time the library is not on the list but the agent is loaded, so that's why it works, but it's not clear to me why this is the case. I think a comment here explaining why would be helpful so the reader doesn't need to track down when each of these "loaded" conditions becomes true.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1213314470