8355960: JvmtiAgentList::Iterator dtor double free with -fno-elide-constructors

[alexmenkov]

Currently jvmtiAgentList keeps agents in reversed order (new agents are added to the head of the list).
To restore original order JvmtiAgentList::Iterator uses GrowableArray allocated in heap.
Iterators for different agent types are returned by value, and the iterator class nas no custom copy ctor, so if the constructor not elides, GrowableArray is deallocated twice.

The fix updates jvmtiAgentList to keep agents in the original order, agents are added to the tail.
Iterator now needs only single pointer to next agent.
Additionally removed JvmtiAgentList::Iterator::next() const method (it looks very strange as next() is expected to change state of the iterator).

Testing: tier1..4,hs-tier5-svc

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8355960: JvmtiAgentList::Iterator dtor double free with -fno-elide-constructors (Bug - P3)

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Serguei Spitsyn (@sspitsyn - Reviewer) Review applies to bf91aef2

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26083/head:pull/26083
$ git checkout pull/26083

Update a local copy of the PR:
$ git checkout pull/26083
$ git pull https://git.openjdk.org/jdk.git pull/26083/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26083

View PR using the GUI difftool:
$ git pr show -t 26083

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26083.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@alexmenkov This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8355960: JvmtiAgentList::Iterator dtor double free with -fno-elide-constructors

Reviewed-by: dholmes, sspitsyn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 36 new commits pushed to the master branch:

• 854de8c: 8336147: Clarify CDS documentation about static vs dynamic archive
• 16af473: 8361115: javax/swing/JComboBox/bug4276920.java unnecessarily throws Error instead of RuntimeException
• da0a51c: 8357601: Checked version of JNI ReleaseArrayElements needs to filter out known wrapped arrays
• ... and 33 more: https://git.openjdk.org/jdk/compare/c6448dc3afb1da9d93bb94804aa1971a650b91b7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@alexmenkov The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 04: Full - Incremental (b1b2dc3f)
• 03: Full - Incremental (bf91aef2)
• 02: Full - Incremental (aeb2b26c)
• 01: Full - Incremental (994ffddc)
• 00: Full (c93b598f)

[dholmes-ora]

I'm not very familiar with these aspects of C++ but this seems to be a very complex solution to what sounds in JBS to be a pretty straight-forward problem.

[dholmes-ora]

You don't need acquire semantics here as you are not doing anything with the _tail pointer value other than use it in relation to cmpxchg which provides fully memory barriers.

[alexmenkov]

ok, thanks

[dholmes-ora]

Why is the tail a pointer to a pointer? Sorry I'm not understanding how your list is being managed here.

I'm trying to work out where you need acquire/release because I'm pretty sure it is missing where needed, but again I can't understand how you are actually constructing and using the list.

[alexmenkov]

pointer to pointer because it contains not the pointer to the last agent in the list, but address of the JvmtiAgent::_next field.
_tail is for faster adding at the end of the list. Performance is not important here and number of agents is always low, so I think _tail can be removed to make the logic simpler. Will update the fix after testing completes

[alexmenkov]

I'm not very familiar with these aspects of C++ but this seems to be a very complex solution to what sounds in JBS to be a pretty straight-forward problem.

Straight-forward solution would be to add copy ctor for Iterator class which clones GrowableArray (allocates new instance and copies all elements).
But I'm not quite happy with the current implementation of AgentList and its iterator, so prefer to update it to keep entries in the original order (and iterator becomes much simpler).
Changes in the callers are required as they used const JvmtiAgentList::Iterator and const next method is removed.

[sspitsyn]

The updated version without _tail looks better. :)
You have some trailing spaces to fix.

[dholmes-ora]

Okay - still took me a little while to understand the double-indirection of the "tail ptr" in add, but I get it now. So looking at the acquire/release requirements:

• all additions are done with cmpxchg which is effectively a release-store
• when you iterate the list all loads of the "next" agent must be a load-acquire, this means
  • when you create the iterator you need a load-acquire (which you have when you pass head() )
  • In Iterator::next() you need a load-acquire on each read of the _next field which is most simply done by defining JvmtiAgent::next() as a load-acquire and using that in the iterator code instead of direct field access.
• JvmtiAgent::set_next should be a release-store though as far as I can see it is not used.

[sspitsyn]

This looks good to me.
I hope, David will sort out where the Atomic:: operations are needed.

[alexmenkov]

Okay - still took me a little while to understand the double-indirection of the "tail ptr" in add, but I get it now. So looking at the acquire/release requirements:

* all additions are done with `cmpxchg` which is effectively a release-store

* when you iterate the list all loads of the "next" agent must be a load-acquire, this means
  
  * when you create the iterator you need a load-acquire (which you have when you pass `head()` )
  * In `Iterator::next()` you need a load-acquire on each read of the `_next` field which is most simply done by defining `JvmtiAgent::next()` as a load-acquire and using that in the iterator code instead of direct field access.

* `JvmtiAgent::set_next` should be a release-store though as far as I can see it is not used.

Thank you for the detailed analysis.
Updated as suggested.

[dholmes-ora]

Looks good. Thanks