8336095: Use-after-free in Superword leads to memory corruption #20297

TobiHartmann · 2024-07-23T10:46:01Z

JDK-8333684 refactored code in Superword and moved a ResourceMark such that a re-allocation of the Node_List IdealLoopTree::_body now happens within a nested ResourceMark. IdealLoopTree::_body is still live and used once we leave the scope of that ResourceMark, leading to a use-after-free bug causing memory corruption (for details, see JBS):

V [libjvm.so+0x13068b6] Node_Array::grow(unsigned int)+0x5e (node.cpp:2776)
V [libjvm.so+0x51e4ec] Node_Array::map(unsigned int, Node*)+0x24 (node.hpp:1621)
V [libjvm.so+0x51e53b] Node_List::push(Node*)+0x33 (node.hpp:1653)
V [libjvm.so+0x149a51f] PhaseIdealLoop::register_new_node(Node*, Node*)+0xf3 (split_if.cpp:440)
V [libjvm.so+0x1197813] PhaseIdealLoop::register_new_node_with_ctrl_of(Node*, Node*)+0x3d (loopnode.hpp:1682)
V [libjvm.so+0x170b1d7] VTransformNode::register_new_node_from_vectorization(VLoopAnalyzer const&, Node*, Node*) const+0x47 (vtransform.cpp:357)
V [libjvm.so+0x170b124] VTransformVectorNode::register_new_node_from_vectorization_and_replace_scalar_nodes(VLoopAnalyzer const&, Node*) const+0x64 (vtransform.cpp:347)
V [libjvm.so+0x170af20] VTransformLoadVectorNode::apply(VLoopAnalyzer const&, GrowableArray<Node*> const&) const+0x244 (vtransform.cpp:322)
V [libjvm.so+0x1564232] VTransformGraph::apply_vectorization_for_each_vtnode(unsigned int&, unsigned int&) const+0xd2 (superword.cpp:2008)
V [libjvm.so+0x15643b0] VTransform::apply_vectorization() const+0x76 (superword.cpp:2029)
V [libjvm.so+0x15637bf] VTransform::apply()+0x201 (superword.cpp:1902)
V [libjvm.so+0x156359a] SuperWord::schedule_and_apply() const+0x15c (superword.cpp:1876)
V [libjvm.so+0x155d90e] SuperWord::SLP_extract()+0x192 (superword.cpp:477)
V [libjvm.so+0x155d705] SuperWord::transform_loop()+0x157 (superword.cpp:392)
V [libjvm.so+0x11ccbed] PhaseIdealLoop::auto_vectorize(IdealLoopTree*, VSharedData&)+0x12d (loopopts.cpp:4448)
V [libjvm.so+0x11af8fb] PhaseIdealLoop::build_and_optimize()+0x1869 (loopnode.cpp:4894)
V [libjvm.so+0x9d4331] PhaseIdealLoop::PhaseIdealLoop(PhaseIterGVN&, LoopOptsMode)+0x163 (loopnode.hpp:1117)
V [libjvm.so+0x9d4599] PhaseIdealLoop::optimize(PhaseIterGVN&, LoopOptsMode)+0x47 (loopnode.hpp:1197)
V [libjvm.so+0x9c46b8] Compile::optimize_loops(PhaseIterGVN&, LoopOptsMode)+0x68 (compile.cpp:2171)
V [libjvm.so+0x9c57c8] Compile::Optimize()+0xf56 (compile.cpp:2418)
V [libjvm.so+0x9be072] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x15ba (compile.cpp:852)

This was reported by SAP (@MBaesken) and only happened in their internal testing. I was able to reproduce this though by adding additional verification code that I will integrate separately with JDK-8336999 because other data structures / containers in C2 should have it as well.

The solution is to allocate IdealLoopTree::_body in the comp_arena (similar to JDK-8325672). My verification code now passes.

Best regards,
Tobias

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-8336095: Use-after-free in Superword leads to memory corruption (Bug - P2)

Reviewers

Emanuel Peter (@eme64 - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)
Richard Reingruber (@reinrich - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20297

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-07-23T10:46:27Z

👋 Welcome back thartmann! 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 · 2024-07-23T10:47:42Z

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

8336095: Use-after-free in Superword leads to memory corruption

Reviewed-by: epeter, kvn, rrich

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

7f11935: 8337167: StringSize deduplication
487450c: 8216471: GTK LnF: Frame is clipped and does not show JTable,Tooltip and JTree demo in SwingSet2 demo
29f0f17: 8336879: Always true condition 'img != null' in GTKPainter.paintPopupMenuBackground
ee839b7: 8337239: Fix simple -Wzero-as-null-pointer-constant warnings in classfile code
0584af2: 8336685: Shenandoah: Remove experimental incremental update mode
8c28f25: 8337124: (fs) sun.nio.fs.WindowsSecurity.enablePrivilege should pin when continuations supported
b5b5a5b: 8336667: IAE in DerInputStream.toByteArray
cf0d9e0: 8337037: compiler internal options are not printing the stacktrace after a compiler crash
81ed028: 8337060: Test java/foreign/TestConcurrentClose.java failed: IllegalStateException: SegmentAccessor::doAccess method not being compiled
e74edba: 8336640: Shenandoah: Parallel worker use in parallel_heap_region_iterate
... and 50 more: https://git.openjdk.org/jdk/compare/c1fdc04ad78e6e4712f2173370012106f9cc45ee...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 · 2024-07-23T10:48:24Z

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

hotspot-compiler

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 · 2024-07-23T10:51:45Z

Webrevs

00: Full (2618e386)

eme64

@TobiHartmann Looks good to me. Thanks for finding the root cause and fixing this bug!

Future RFE:
We should probably create a dedicated arena for each Phase, so that the memory can be released after each phase, and re-used properly with the next phase. That could reduce memory footprint.

TobiHartmann · 2024-07-23T11:31:59Z

Thanks for the review.

We should probably create a dedicated arena for each Phase, so that the memory can be released after each phase, and re-used properly with the next phase. That could reduce memory footprint.

Yes, I thought about that as well. There are also some places where we use the comp_arena but resoure arena could be used. I'll file an RFE.

TobiHartmann · 2024-07-23T11:58:32Z

Filed JDK-8337015.

reinrich · 2024-07-23T14:46:30Z

I tried but could not reproduce the issues with your fix. They do reproduce with baseline.
Nighly tests are pending.

vnkozlov

Good.

TobiHartmann · 2024-07-24T05:21:00Z

Thanks Richard and Vladimir. I'll wait for final confirmation from @reinrich before integrating this.

MBaesken · 2024-07-24T07:53:24Z

The nightly test runs of the suite triggering those issues were good, but probably we have to wait 1-2 more days (the issue did not show up every night).

MBaesken · 2024-07-26T07:50:11Z

The nightly test runs of the suite triggering those issues were good, but probably we have to wait 1-2 more days (the issue did not show up every night).

No issues seen in our test infrastructure with the patch for a couple of days. Looks good (but I am not a JIT expert) .

reinrich

The fix looks good to me.
Thanks, Richard.

TobiHartmann · 2024-07-26T13:47:03Z

Great, thanks for checking! I'll integrate on Monday.

TobiHartmann · 2024-07-29T05:03:42Z

/integrate

openjdk · 2024-07-29T05:05:35Z

Going to push as commit 90641a6.
Since your change was applied there have been 72 commits pushed to the master branch:

2fbdbac: 8337245: Fix wrong comment of StringConcatHelper
034297a: 8336240: Test com/sun/crypto/provider/Cipher/DES/PerformanceTest.java fails with java.lang.ArithmeticException
abc4ca5: 8330427: Obsolete -XX:+PreserveAllAnnotations
0c3720b: 8335131: Test "javax/swing/JColorChooser/Test6977726.java" failed on ubuntu x64 because "Preview" title is missing for GTK L&F
4f194f1: 8337241: Shenandoah: Normalize include guards
f35af71: 8334315: Shenandoah: reduce GC logging noise
4bcb8f0: 8337243: Fix more -Wzero-as-null-pointer-constant warnings in compiler code
3abe8a6: 8336663: [JVMCI] VM Crash on ZGC due to incompatible handle returned by HotSpotJVMCIRuntime#getJObjectValue
5ff7c57: 8337168: Optimize LocalDateTime.toString
374fca0: 8335967: "text-decoration: none" does not work with "A" HTML tags
... and 62 more: https://git.openjdk.org/jdk/compare/c1fdc04ad78e6e4712f2173370012106f9cc45ee...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-29T05:05:41Z

@TobiHartmann Pushed as commit 90641a6.

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

8336095: Use-after-free in Superword leads to memory corruption

2618e38

openjdk bot added the rfr Pull request is ready for review label Jul 23, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 23, 2024

eme64 approved these changes Jul 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 23, 2024

vnkozlov approved these changes Jul 23, 2024

View reviewed changes

reinrich approved these changes Jul 26, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 29, 2024

openjdk bot closed this Jul 29, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 29, 2024

8336095: Use-after-free in Superword leads to memory corruption #20297

8336095: Use-after-free in Superword leads to memory corruption #20297

Uh oh!

Conversation

TobiHartmann commented Jul 23, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jul 23, 2024

Uh oh!

openjdk bot commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 23, 2024

Uh oh!

mlbridge bot commented Jul 23, 2024

Webrevs

Uh oh!

eme64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jul 23, 2024

Uh oh!

TobiHartmann commented Jul 23, 2024

Uh oh!

reinrich commented Jul 23, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jul 24, 2024

Uh oh!

MBaesken commented Jul 24, 2024

Uh oh!

MBaesken commented Jul 26, 2024

Uh oh!

reinrich left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jul 26, 2024

Uh oh!

TobiHartmann commented Jul 29, 2024

Uh oh!

openjdk bot commented Jul 29, 2024

Uh oh!

openjdk bot commented Jul 29, 2024

Uh oh!

Uh oh!

TobiHartmann commented Jul 23, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jul 23, 2024 •

edited

Loading

eme64 left a comment •

edited

Loading