-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8369506: Bytecode rewriting causes Java heap corruption on AArch64 #27748
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
Conversation
👋 Welcome back jcking! A progress list of the required criteria for merging this PR into |
@jcking 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:
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 2 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
/contributor add manc |
/contributor add rasbold |
@jcking |
@jcking |
Signed-off-by: Justin King <[email protected]>
cd74eac
to
5b5087e
Compare
How does this follow? We need some sort of happens-before relationship on the reader side to make sure that the resolved field entry is observed. I guess this PR relies on a control dependency between reading the patched bytecode and executing the code that reads the resolved field entry. |
Yes, that is what the PR relies upon. However we are still discussing internally on whether that is enough as we would rather not have a repeat N years down the line as hardware advances. I left this in draft until we figure it out and will poke you once we are more confident. The AArch64 docs are not super clear. It does have this sentence: |
B2.3.6, Dependency relations, Control dependency, gives you what you need on the reader side. |
Based on conversations in https://bugs.openjdk.org/browse/JDK-8369506 and herd7 models, it is believed that this patch is sufficient for AArch64. Marking as ready. |
@shipilev PTAL as well. |
Co-authored-by: Andrew Haley <[email protected]>
Signed-off-by: Justin King <[email protected]>
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree this is sufficient. I have only cosmetic comments:
/reviewers 2 |
Signed-off-by: Justin King <[email protected]>
Now running tests in my Graviton 3 instance to make sure the new verification code is not barfing up anywhere. PR patch applies with fuzz, BTW, consider merging from mainline:
|
Merged. |
Linux AArch64 server fastdebug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Please make sure that this has bug reports for PPC and RISCV, and announce them to hotspot-dev.
When you talk about announce, are you just referring to sending a somewhat short and sweet email to hotspot-dev calling out the 3 issues (RISCV and PPC bugs to be created) and that they can result in Java heap corruption? Just want to make sure I am not annoying people. I'll file the RISCV and PPC bugs shortly or tomorrow morning, integrate this after, and pursue a backport to JDK 25. There was a code change in this path before JDK 25, IIRC, so the fix would have to be validated again for JDKs before that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, but with a question about hoisting the member(LoadLoad)
:
https://bugs.openjdk.org/browse/JDK-8369506?focusedId=14825240&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14825240
There is no need to announce on hotspot-dev. I looked through the other arches, so:
|
One can, but then you'll have to remember to put it in every |
@theRealAph -- oddly, I have GH notifications about some of your recent suggestions in comments, but I do not see them in PR itself. Maybe you need to "publish" the review? |
No, I made some mistakes so deleted the comments. |
/integrate |
Going to push as commit 18fd047.
Your commit was automatically rebased without conflicts. |
Fix JDK-8369506 by adding
STLR
when updating the bytecode. Additionally I added a quick debug only check which verifies the field offset we get fromResolvedFieldEntry
inTemplateTable::fast_*
will not clobber the header or Klass pointer. The addedSTLR
, a long with the already existingDMB ISHLD
inInterpreterMacroAssembler::load_field_entry
, guarantees that the fully filled outResolvedFieldEntry
is observable if the patched bytecode is observable. We do not need to addLDAR
for bytecode loading orLDAR
inTemplateTable::fast_*
for that reason. If another observer happens to observe a0
field offset, its guaranteed then that they will also observe the non-patched bytecode which will ultimately end up doing the resolution again, which is okay.Progress
Issue
Reviewers
Contributors
<[email protected]>
<[email protected]>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27748/head:pull/27748
$ git checkout pull/27748
Update a local copy of the PR:
$ git checkout pull/27748
$ git pull https://git.openjdk.org/jdk.git pull/27748/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27748
View PR using the GUI difftool:
$ git pr show -t 27748
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27748.diff
Using Webrev
Link to Webrev Comment