Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject #19439

Closed
wants to merge 4 commits into from

Conversation

plummercj
Copy link
Contributor

@plummercj plummercj commented May 28, 2024

Fixed "double-checked-locking" bug in ReferenceImplType.classObject(). Details in the first comment. Tested with the following:

  • com/sun/jdi
  • nsk/jdi
  • nsk/jdwp
  • nsk/jdb

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-8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject (Bug - P5)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19439

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

Using diff file

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

Webrev

Link to Webrev Comment

@plummercj
Copy link
Contributor Author

plummercj commented May 28, 2024

The bug is the classic "double-checked-locking" flaw. In general "double-checked-locking" does not work, but in this case, based on how it is used and the java memory model, it can be made to work by making the field volatile. Although the issue could be fixed by making the classObject field volatile, I decided to get rid of the double-checked-locking instead. There is little benefit to using it here (it simply avoids fetching the info twice when there is a race, which is very rare), and leaving it out simplifies the code and reduces overhead for the usual case (when there is no race). Regarding the pre-existing comment:

        // Are classObjects unique for an Object, or
        // created each time? Is this spec'ed?

Yes, they are unique. No they are not created each time. Probably this lack of understanding is why double-checked-locking was used here. The ObjectReference spec is a bit loose on the wording, referring to an ObjectReference as "An object that currently exists in the target VM". However, the implementation is not. VirtualMachineImpl.objectMirror() is used to create all ObjectReference instances, and it only creates one ObjectReference per JDWP objectID. I tested this by making classObject() fetch the ObjectReference on every call and compare the result to the cached value, and they always match. Also, the JDWP spec calls out that each java Object instance has 1 unique objectID. The following is the JDWP spec description of the objectID type.

"Uniquely identifies an object in the target VM. A particular object will be identified by exactly one objectID in JDWP commands and replies throughout its lifetime (or until the objectID is explicitly disposed). An ObjectID is not reused to identify a different object unless it has been explicitly disposed, regardless of whether the referenced object has been garbage collected. An objectID of 0 represents a null object. Note that the existence of an object ID does not prevent the garbage collection of the object. Any attempt to access a a garbage collected object with its object ID will result in the INVALID_OBJECT error code. Garbage collection can be disabled with the DisableCollection command, but it is not usually necessary to do so."

While looking into this bug I also ran across the similar classLoader() API, and noticed that it did not use synchronized and explained why in a comment. That is where I got the motivation to remove synchronized from classObject(). Then I found a bunch more similar APIs. I cleaned up their comment to be consistent.

@bridgekeeper
Copy link

bridgekeeper bot commented May 28, 2024

👋 Welcome back cjplummer! 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
Copy link

openjdk bot commented May 28, 2024

@plummercj 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:

8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject

Reviewed-by: amenkov, 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 155 new commits pushed to the master branch:

  • 6d718ae: 8324341: Remove redundant preprocessor #if's checks
  • 9b64ece: 8332904: ubsan ppc64le: c1_LIRGenerator_ppc.cpp:581:21: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'
  • 3d4eb15: 8302744: Refactor Hotspot container detection code
  • 2cca83b: 8332880: JFR GCHelper class recognizes "Archive" regions as valid
  • b8ae11e: 8332960: ubsan: classListParser.hpp:159:12: runtime error: load of value 2101478704, which is not a valid value for type 'ParseMode'
  • 9a83dfe: 8332431: NullPointerException in JTable of SwingSet2
  • 01060ad: 8325083: jdk/incubator/vector/Double512VectorTests.java crashes in Assembler::vex_prefix_and_encode
  • 673f767: 8285506: Unify os::vsnprintf implementations
  • 91ab088: 8333116: test/jdk/tools/jpackage/share/ServiceTest.java test fails
  • 9ac8d05: 8332228: TypePollution.java: Unrecognized VM option 'UseSecondarySuperCache'
  • ... and 145 more: https://git.openjdk.org/jdk/compare/fa3e94d30f11bdccbe290041ae19490ce4940bb1...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 openjdk bot changed the title 8328611 8328611: Thread safety issue in com.sun.tools.jdi.ReferenceTypeImpl::classObject May 28, 2024
@openjdk openjdk bot added the rfr Pull request is ready for review label May 28, 2024
@openjdk
Copy link

openjdk bot commented May 28, 2024

@plummercj The following label will be automatically applied to this pull request:

  • serviceability

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

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label May 28, 2024
@mlbridge
Copy link

mlbridge bot commented May 28, 2024

Webrevs

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 29, 2024
@plummercj
Copy link
Contributor Author

Thanks for the reviews Serguei and Alex!

/integrate

@openjdk
Copy link

openjdk bot commented May 30, 2024

Going to push as commit 922e312.
Since your change was applied there have been 179 commits pushed to the master branch:

  • 1d889e5: 8332487: Regression in Crypto-AESGCMBench.encrypt (and others) after JDK-8328181
  • 32636dc: 8333105: Shenandoah: Results of concurrent mark may be lost for degenerated cycle
  • 7071542: 8331189: Implementation of Scoped Values (Third Preview)
  • 4acafb8: 8333107: javac fails with an exception when processing broken lambda
  • 921860d: 8333264: Remove unused resolve_sub_helper declaration after JDK-8322630
  • 4a20691: 8331876: JFR: Move file read and write events to java.base
  • f608918: 8332614: Type-checked ConstantPool.entryByIndex and ClassReader.readEntryOrNull
  • 1b04f64: 8216984: Deprecate for removal Socket constructors to create UDP sockets
  • 3cff588: 8332826: Make hashCode methods in ArraysSupport friendlier
  • 2b4a4b7: 8326121: vmTestbase/gc/g1/unloading/tests/unloading_keepRef_rootClass_inMemoryCompilation_keep_cl failed with Full gc happened. Test was useless.
  • ... and 169 more: https://git.openjdk.org/jdk/compare/fa3e94d30f11bdccbe290041ae19490ce4940bb1...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 30, 2024
@openjdk openjdk bot closed this May 30, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 30, 2024
@openjdk
Copy link

openjdk bot commented May 30, 2024

@plummercj Pushed as commit 922e312.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

3 participants