8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp #16405

matias9927 · 2023-10-27T15:40:58Z

Calls in instanceKlass.cpp and unsafe.cpp try to call an atomic load on method calls that could return nullptr. This patch ensures that nullptr is not passed into the load.

In print_as_native_pointer in archiveBuilder, source_obj_to_requested_obj should not be able to return nullptr as the result is immediately cast to an oop which cascades down to the failure reported in get_volatile() in unsafe.cpp. Placing an assert close to the top of this call stack should prevent this from happening and will better indicate the source of an unexpected nullptr should it occur.

Verified with tier1-5 tests.

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-8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp (Bug - P3)

Reviewers

Coleen Phillimore (@coleenp - Reviewer)
Calvin Cheung (@calvinccheung - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16405

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

Using diff file

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

Webrev

Link to Webrev Comment

…e.cpp

bridgekeeper · 2023-10-27T15:42:43Z

👋 Welcome back matsaave! 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 · 2023-10-27T15:43:21Z

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

hotspot

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.

mlbridge · 2023-10-27T16:44:50Z

Webrevs

01: Full - Incremental (0c91778f)
00: Full (a546415a)

coleenp

Looks good.

openjdk · 2023-10-27T18:17:18Z

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

8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp

Reviewed-by: coleenp, ccheung, dholmes

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 275 new commits pushed to the master branch:

b3fec6b: 8306980: Generated docs should contain correct Legal Documents
1139482: 8316132: CDSProtectionDomain::get_shared_protection_domain should check for exception
2182c93: 8313643: Update HarfBuzz to 8.2.2
613a3cc: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library
613d32c: 8169475: WheelModifier.java fails by timeout
f1e8787: 8317609: Classfile API fails to verify /jdk.jcmd/sun/tools/jstat/Alignment.class
47624f6: 8299058: AssertionError in sun.net.httpserver.ServerImpl when connection is idle
2d5829a: 8239508: JFR: @RemoveFields
0064cf9: 8311596: Add separate system properties for TLS server and client for maximum chain length
3a7525d: 8309118: HttpClient: Add more tests for 100 ExpectContinue with HTTP/2
... and 265 more: https://git.openjdk.org/jdk/compare/77fa44fd4fefbc9ba41a2ed4bd931c326e5255e4...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.

calvinccheung

LGTM

dholmes-ora · 2023-10-30T07:36:19Z

src/hotspot/share/oops/instanceKlass.cpp

+      InstanceKlass* volatile* iklass = adr_implementor();
+      InstanceKlass* impl = (iklass != nullptr) ? Atomic::load_acquire(iklass) : nullptr;


This looks very klunky as we do a raw read, check it for null then re-read with acquire semantics. Cleaner IMO to do a raw read followed by a raw OrderAccess::acquire() and no need for a null check.

dholmes-ora · 2023-10-30T07:40:10Z

src/hotspot/share/prims/unsafe.cpp

@@ -231,6 +231,7 @@ class MemoryAccess : StackObj {

  T get_volatile() {
    GuardUnsafeAccess guard(_thread);
+    assert(addr() != nullptr, "Attempting to load from null pointer");


I don't see how addr() can be null unless _obj was null - which would be a usage error. So asserting _obj != nullptr in the constructor would seem better to me. I mean no point checking addr() here but not in other functions where we dereference it!

coleenp

Yes, this looks better. This was the source of the nullptr, except in these two cases, the pointer is never null.

calvinccheung

Updated changes look good.

dholmes-ora

Not sure why it needed to be lifted out of Unsafe. The issue description should be updated now.

matias9927 · 2023-10-31T14:59:50Z

Not sure why it needed to be lifted out of Unsafe. The issue description should be updated now.

Sorry, I meant to explain this change in response to your comments. I'll explain here:

The call stack that results in the issue shown in get_volatile() starts near where the new assert is placed. When discussing with @coleenp, we decided that placing the assert at the source of the nullptr would be a better indicate the problem should it arise in the code. The description has been updated with more detail.

dholmes-ora

Okay. Thanks

matias9927 · 2023-11-02T14:28:37Z

Thanks for the reviews @coleenp , @calvinccheung , and @dholmes-ora!
/integrate

openjdk · 2023-11-02T14:30:44Z

Going to push as commit 7a7b1e5.
Since your change was applied there have been 301 commits pushed to the master branch:

cb20a3e: 8319166: Typos in the JavaDocs for MemorySegment
99efcde: 8317545: AIX PPC64: Implementation of Foreign Function & Memory API
e9d19d0: 8319300: Remove unused methods in WorkArounds and Utils
faa8bde: 8319206: [REDO] Event NativeLibraryLoad breaks invariant by taking a stacktrace when thread is in state _thread_in_native
4f808c6: 8316538: runtime/handshake/MixedHandshakeWalkStackTest.java crashes with JFR
2d4a4d0: 8315921: Invalid CSS declarations in java.lang class documentation
792d829: 8319205: Parallel: Reenable work stealing after JDK-8310031
23a96bf: 8318894: G1: Use uint for age in G1SurvRateGroup
64f8253: 8317332: Prepare security for permissive-
53bb7cd: 8318957: enhance agentlib:jdwp help output by info about allow option
... and 291 more: https://git.openjdk.org/jdk/compare/77fa44fd4fefbc9ba41a2ed4bd931c326e5255e4...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-02T14:30:51Z

@matias9927 Pushed as commit 7a7b1e5.

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

8315890: Attempts to load from nullptr in instanceKlass.cpp and unsaf…

a546415

…e.cpp

openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 27, 2023

matias9927 marked this pull request as ready for review October 27, 2023 16:38

openjdk bot added the rfr Pull request is ready for review label Oct 27, 2023

coleenp approved these changes Oct 27, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 27, 2023

calvinccheung approved these changes Oct 28, 2023

View reviewed changes

dholmes-ora suggested changes Oct 30, 2023

View reviewed changes

Moved assert higher in call stack

0c91778

coleenp approved these changes Oct 30, 2023

View reviewed changes

calvinccheung approved these changes Oct 30, 2023

View reviewed changes

dholmes-ora reviewed Oct 30, 2023

View reviewed changes

dholmes-ora approved these changes Nov 1, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 2, 2023

openjdk bot closed this Nov 2, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp #16405

8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp #16405

matias9927 commented Oct 27, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

mlbridge bot commented Oct 27, 2023 •

edited

Loading

coleenp left a comment

openjdk bot commented Oct 27, 2023 •

edited

Loading

calvinccheung left a comment

dholmes-ora Oct 30, 2023

dholmes-ora Oct 30, 2023

coleenp left a comment •

edited

Loading

calvinccheung left a comment

dholmes-ora left a comment

matias9927 commented Oct 31, 2023

dholmes-ora left a comment

matias9927 commented Nov 2, 2023

openjdk bot commented Nov 2, 2023

openjdk bot commented Nov 2, 2023

		InstanceKlass* volatile* iklass = adr_implementor();
		InstanceKlass* impl = (iklass != nullptr) ? Atomic::load_acquire(iklass) : nullptr;

8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp #16405

8315890: Attempts to load from nullptr in instanceKlass.cpp and unsafe.cpp #16405

Conversation

matias9927 commented Oct 27, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 27, 2023

openjdk bot commented Oct 27, 2023

mlbridge bot commented Oct 27, 2023 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 27, 2023 • edited Loading

calvinccheung left a comment

Choose a reason for hiding this comment

dholmes-ora Oct 30, 2023

Choose a reason for hiding this comment

dholmes-ora Oct 30, 2023

Choose a reason for hiding this comment

coleenp left a comment • edited Loading

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

matias9927 commented Oct 31, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

matias9927 commented Nov 2, 2023

openjdk bot commented Nov 2, 2023

openjdk bot commented Nov 2, 2023

matias9927 commented Oct 27, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 27, 2023 •

edited

Loading

openjdk bot commented Oct 27, 2023 •

edited

Loading

coleenp left a comment •

edited

Loading