JDK-8302670 : use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 11,17,21
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2023-02-16
  • Updated: 2025-06-04
  • Resolved: 2023-05-30
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 21
21 b25Fixed
Related Reports
Causes :  
JDK-8358598 - PhaseIterGVN::PhaseIterGVN(PhaseGVN* gvn) doesn't use its parameter
Relates :  
JDK-8337015 - Revisit resource arena allocations in C2
Relates :  
JDK-8319649 - inline_boxing_calls unused gvn variable
Relates :  
JDK-8301373 - Potential use after free in GrowableArray
Relates :  
JDK-8307815 - C2 Phase structure cleanup
Relates :  
JDK-8325672 - C2: allocate PhaseIdealLoop::_loop_or_ctrl from C->comp_arena()
Description
Instrumenting Arena for ASan has revealed an awkward use-after-free in C2. It appears PhaseIterGVN ends up copying Compile::for_igvn() by value in its constructor. It then either intentionally or unintentionally causes the list to grow triggering Arealloc. This leaves the original Compile::for_igvn() Unique_Node_List now pointing to free'd memory which eventually gets used again.

We need to detangle the expectation with PhaseIterGVN and whether modifications to either Unique_Node_List or Node_Stack it copies by value in its constructors should be visible to the caller.

Should PhaseIterGVN be using copy-on-write semantics? Should PhaseIterGVN be propagating changes to either Unique_Node_List or Node_Stack back to the caller it copied it from?

==3227315==ERROR: AddressSanitizer: use-after-poison on address 0x6290005b9210 at pc 0x7f619ca47681 bp 0x7f60d80eb8e0 sp 0x7f60d80eb090                                                                                                                                           [105/1351]
WRITE of size 16384 at 0x6290005b9210 thread T17
    #0 0x7f619ca47680 in __interceptor_memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:799
    #1 0x7f6197e15ccb in Copy::pd_zero_to_bytes(void*, unsigned long) src/hotspot/cpu/x86/copy_x86.hpp:59
    #2 0x7f6197e15ccb in Copy::zero_to_bytes(void*, unsigned long) src/hotspot/share/utilities/copy.hpp:298
    #3 0x7f6197e15ccb in Node_Array::clear() src/hotspot/share/opto/node.hpp:1548
    #4 0x7f6197e15ccb in Node_List::clear() src/hotspot/share/opto/node.hpp:1572
    #5 0x7f6197e15ccb in Unique_Node_List::clear() src/hotspot/share/opto/node.hpp:1623
    #6 0x7f6197e15ccb in Compile::Optimize() src/hotspot/share/opto/compile.cpp:2269
    #7 0x7f6197e1cf12 in Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*) src/hotspot/share/opto/compile.cpp:833
    #8 0x7f6197af05ce in C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*) src/hotspot/share/opto/c2compiler.cpp:113
    #9 0x7f6197e33890 in CompileBroker::invoke_compiler_on_method(CompileTask*) src/hotspot/share/compiler/compileBroker.cpp:2237
    #10 0x7f6197e364a7 in CompileBroker::compiler_thread_loop() src/hotspot/share/compiler/compileBroker.cpp:1916
    #11 0x7f61989b235b in JavaThread::thread_main_inner() src/hotspot/share/runtime/javaThread.cpp:710
    #12 0x7f61989b274f in JavaThread::thread_main_inner() src/hotspot/share/runtime/javaThread.cpp:689
    #13 0x7f61989b274f in JavaThread::run() src/hotspot/share/runtime/javaThread.cpp:695
    #14 0x7f619a1f0e75 in Thread::call_run() src/hotspot/share/runtime/thread.cpp:224
    #15 0x7f619986317f in thread_native_entry src/hotspot/os/linux/os_linux.cpp:737
    #16 0x7f619c8a7fd3 in start_thread nptl/pthread_create.c:442
    #17 0x7f619c92866b in clone3 ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

0x6290005b9210 is located 16 bytes inside of 16400-byte region [0x6290005b9200,0x6290005bd210)
allocated by thread T17 here:
    #0 0x7f619cab89cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
    #1 0x7f619984b0ba in os::malloc(unsigned long, MEMFLAGS, NativeCallStack const&) src/hotspot/share/runtime/os.cpp:673
    #2 0x7f619749a87e in Chunk::operator new(unsigned long, AllocFailStrategy::AllocFailEnum, unsigned long) src/hotspot/share/memory/arena.cpp:190
    #3 0x7f619749a87e in Arena::grow(unsigned long, AllocFailStrategy::AllocFailEnum) src/hotspot/share/memory/arena.cpp:319
    #4 0x7f619749ac5c in Arena::internal_amalloc(unsigned long, AllocFailStrategy::AllocFailEnum) src/hotspot/share/memory/arena.hpp:113
    #5 0x7f619749ac5c in Arena::Amalloc(unsigned long, AllocFailStrategy::AllocFailEnum) src/hotspot/share/memory/arena.hpp:133
    #6 0x7f619749ac5c in Arena::Arealloc(void*, unsigned long, unsigned long, AllocFailStrategy::AllocFailEnum) src/hotspot/share/memory/arena.cpp:370
    #7 0x7f61997a5f6f in Node_Array::grow(unsigned int) src/hotspot/share/opto/node.cpp:2778
    #8 0x7f61998fd96c in Node_Array::map(unsigned int, Node*) src/hotspot/share/opto/node.hpp:1543
    #9 0x7f61998fd96c in Node_List::push(Node*) src/hotspot/share/opto/node.hpp:1569
    #10 0x7f61998fd96c in Unique_Node_List::push(Node*) src/hotspot/share/opto/node.hpp:1601
    #11 0x7f61998fd96c in Compile::record_for_igvn(Node*) src/hotspot/share/opto/node.hpp:1672
    #12 0x7f61998fd96c in GraphKit::record_for_igvn(Node*) const src/hotspot/share/opto/graphKit.hpp:96
    #13 0x7f61998fd96c in Parse::merge_memory_edges(MergeMemNode*, int, bool) src/hotspot/share/opto/parse1.cpp:1886
    #14 0x7f61998fe585 in Parse::merge_common(Parse::Block*, int) src/hotspot/share/opto/parse1.cpp:1763
    #15 0x7f6199902b6e in Parse::do_all_blocks() src/hotspot/share/opto/parse1.cpp:707
    #16 0x7f619990c6b4 in Parse::Parse(JVMState*, ciMethod*, float) src/hotspot/share/opto/parse1.cpp:613
    #17 0x7f6197af3175 in ParseGenerator::generate(JVMState*) src/hotspot/share/opto/callGenerator.cpp:99
    #18 0x7f6197af9b11 in PredictedCallGenerator::generate(JVMState*) src/hotspot/share/opto/callGenerator.cpp:915
    #19 0x7f61981ff789 in Parse::do_call() src/hotspot/share/opto/doCall.cpp:662
    #20 0x7f619992e27f in Parse::do_one_bytecode() src/hotspot/share/opto/parse2.cpp:2704
    #21 0x7f6199901796 in Parse::do_one_block() src/hotspot/share/opto/parse1.cpp:1560
    #22 0x7f6199902b6e in Parse::do_all_blocks() src/hotspot/share/opto/parse1.cpp:707
    #23 0x7f619990c6b4 in Parse::Parse(JVMState*, ciMethod*, float) src/hotspot/share/opto/parse1.cpp:613
    #24 0x7f6197af3175 in ParseGenerator::generate(JVMState*) src/hotspot/share/opto/callGenerator.cpp:99
    #25 0x7f6197af9b11 in PredictedCallGenerator::generate(JVMState*) src/hotspot/share/opto/callGenerator.cpp:915
    #26 0x7f61981ff789 in Parse::do_call() src/hotspot/share/opto/doCall.cpp:662
    #27 0x7f619992e27f in Parse::do_one_bytecode() src/hotspot/share/opto/parse2.cpp:2704
    #28 0x7f6199901796 in Parse::do_one_block() src/hotspot/share/opto/parse1.cpp:1560
    #29 0x7f6199902b6e in Parse::do_all_blocks() src/hotspot/share/opto/parse1.cpp:707
    #30 0x7f619990c6b4 in Parse::Parse(JVMState*, ciMethod*, float) src/hotspot/share/opto/parse1.cpp:613
    #31 0x7f6197af3175 in ParseGenerator::generate(JVMState*) src/hotspot/share/opto/callGenerator.cpp:99
    #32 0x7f6197e1c9cc in Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*) src/hotspot/share/opto/compile.cpp:763
    #33 0x7f6197af05ce in C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*) src/hotspot/share/opto/c2compiler.cpp:113
    #34 0x7f6197e33890 in CompileBroker::invoke_compiler_on_method(CompileTask*) src/hotspot/share/compiler/compileBroker.cpp:2237
    #35 0x7f6197e364a7 in CompileBroker::compiler_thread_loop() src/hotspot/share/compiler/compileBroker.cpp:1916
    #36 0x7f61989b235b in JavaThread::thread_main_inner() src/hotspot/share/runtime/javaThread.cpp:710
    #37 0x7f61989b274f in JavaThread::thread_main_inner() src/hotspot/share/runtime/javaThread.cpp:689
    #38 0x7f61989b274f in JavaThread::run() src/hotspot/share/runtime/javaThread.cpp:695
Comments
Changeset: 1f1f6040 Author: Emanuel Peter <epeter@openjdk.org> Date: 2023-05-30 07:14:50 +0000 URL: https://git.openjdk.org/jdk/commit/1f1f604071dc2fca4849eb6ce251d5f18b443e16
30-05-2023
A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/13833 Date: 2023-05-05 14:08:42 +0000
10-05-2023
[~jcking] Thanks for your PR. I looked at the problem, and your suggested changes. Currently, we really have a tangled mess. I like the idea of having clear ownership of the Node_List, etc. So making them non-copyable is a good start, that way we at least do not have inconsistent states because of internals being copied by value. For the "Phases", we have some internal data-structures that get passed from phase to phase. I see that you made the IGVN "_worklist" live in "Compile", and use it as reference everywhere else. I like that. Can we do something similar with the "_nodes" and "_types" for "PhaseTransform"? I'm not sure, but it might be that we also only need one of each of those per compilation (with the exception of the verification loop-opts). I understand why you use the "replace_with" method: you want to move the internals of the source to the destination, and then remove the ownership of the source, by nullptr-ing out the memory references. This is nice, we only have one owner of the internally allocated memory. However, it just leaves the source as dead. What should happen if we use the source again? Throw an assert or hit a nullptr-dereference / SIGSEGV? Or should it just be empty, and re-allocate memory on insert, as you do with "Node_Array::insert"? In "PhaseTransform::PhaseTransform( PhaseTransform *pt, PhaseNumber pnum )", you initialize "_nodes" and "_types" with the "Node_List::Node_List(Node_List *nl)" constructor. This calls "replace_with", and invalidates/empties the "_nodes" and "_types" of the input "pt". I'm not sure that the user of the "PhaseTransform" (or subclass) constructor is aware that the internals will be moved over to the new phase, and the old phase is invalid/empty now. I wonder if it would be smarter to keep the "_nodes" and "_types" outside the phase, and only use them as references inside? We could keep them in the "Compile", or generate new ones temporarily if a "PhaseTransform" needs to have a new set of those data-structures. That way, the owner is clear ("Compile", or some outer context owns it). And the owner will have a wider scope than the reference-uses: we will only pass them downward in the call hierarchy.
25-04-2023
I have https://github.com/openjdk/jdk/pull/12703, but I am *really* not confident in it so I have no intention on taking it past a draft. Just dropping here as a discussion or starting point for somebody else. It makes phases and all the containers non-copyable. It uses some implicit moves (rvalues) to allow returning containers, some move-like functions (replace_with), and reset. This ensures only one container actually owns it at a single time. In this case, C2 might benefit from using rvalues and move constructors/assignments even though Hotspot in general has not decided on their usage.
21-02-2023
Emanuel, could you please have a look at this?
20-02-2023
Right, that's a good suggestion.
20-02-2023
I think the way to go about fixing this is probably to start by adding NONCOPYABLE to Type_Array, Node_Stack, Node_Array, NodeHash, and friends. This will trigger compile errors that will identify where the odd copying is being performed. Then it's a matter of determining whether it should be by reference or whether it should probably be a move.
17-02-2023
ILW = Use-after-free in C2 code, most likely not an issue with current code but could affect memory consumption, no workaround = MMH = P3
17-02-2023
Nice analysis, thanks for the report! C->for_igvn() is used to record nodes during parsing (GVN) that should be put on the IGVN worklist later. Once IGVN is over, the for_igvn() node list is cleared and re-used when inlining (parsing) again incrementally. > It then either intentionally or unintentionally causes the list to grow triggering Arealloc Since C->for_igvn() is passed by value to PhaseIterGVN::_worklist but the underlying Node_Array::_nodes memory is shared, I would assume that a growing _worklist during IGVN, which is expected, affects C->for_igvn() as well, right? > This leaves the original Compile::for_igvn() Unique_Node_List now pointing to free'd memory which eventually gets used again. Right, I think the only reason that this does not cause any real problems is that a free in the Arena (Arena::Afree) is a NOP. We just drop the memory and allocate a new chunk. If C->for_igvn() still refers to it, it's guaranteed to not be re-used by something else. I think C->for_igvn() should be "owned" by Compile and passed by reference to IGVN such that the same node list is re-used by all the parsing runs. This could also reduce overall memory consumption because we potentially don't need to re-allocate that often.
17-02-2023