JDK-8301373 : Potential use after free in GrowableArray
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 11,17,21
  • Priority: P3
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2023-01-30
  • Updated: 2024-08-08
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.
Other
tbdUnresolved
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Sub Tasks
JDK-8309203 :  
Description
Consider the following code:

    Foo f;
    GrowableArray<Foo> arr(1, 1, f); // contains 1 copy of 'f'
    GrowableArray arr2(arr); // copy constructed

    arr.push(f); // add another copy of f

    Foo* foo_adr = arr2.adr_at(0); // reference to first element
    // or: Foo& foo_ref = arr2.at(0);

    foo_adr->do_something(); // BOOM!

Note that `arr` and `arr2` share a pointer to the underlying `_data` array.

When the call to 'push' happens, the capacity of the array is not big enough for the new element. This means that a new array is allocated, the elements copied to the new array, and the destructor is called for each element in the old array.

But, the data pointer of arr2 is not updated. This means that `foo_adr` will point at a destroyed object.

This can be prevented by making either:
- Making GrowableArray NONCOPYABLE
- Copying the _data array when the GrowableArray object is copied, so that `arr` and `arr2` in the example will have a distinct set of elements.
Comments
I'm unassigning this for now. I had worked on GrowableArray for a while, but then broader work was discussed, and I backed away from this project. Feel free to reach out to me if you have any questions.
08-08-2024

One of the really big problems is currently that GrowableArray cannot be made NOCOPYABLE because of recursive uses like this: BlockBegin* BlockListBuilder::make_block_at(int cur_bci, BlockBegin* predecessor) { assert(method()->bci_block_start().at(cur_bci), "wrong block starts of MethodLivenessAnalyzer"); BlockBegin* block = _bci2block->at(cur_bci); if (block == nullptr) { block = new BlockBegin(cur_bci); block->init_stores_to_locals(method()->max_locals()); _bci2block->at_put(cur_bci, block); _bci2block_successors.at_put_grow(cur_bci, BlockList()); _bci2block_successors is basically a GrowableArray<GrowableArray<something>>. And GrowableArray internally uses the copy-constructor to move around elements. This is because we do not yet allow move-semantics. Hence, we need the copy constructor for GrowableArray, if it is to be allowed as an inner array. But all of this is very problematic. For example this following patch to the test fails: diff --git a/test/hotspot/gtest/utilities/test_growableArray.cpp b/test/hotspot/gtest/utilities/test_growableArray.cpp index cd269e09212..ca0b341ad7f 100644 --- a/test/hotspot/gtest/utilities/test_growableArray.cpp +++ b/test/hotspot/gtest/utilities/test_growableArray.cpp @@ -434,6 +434,16 @@ class GrowableArrayTest : public ::testing::Test { static void with_no_cheap_array_append1(TestNoCHeapEnum test) { with_no_cheap_array(Max0, Append1, test); } + + static void test_inner_array() { + ResourceMark rm; + GrowableArray<GrowableArray<int>> outer; + outer.at_grow(100); + outer.at(0).at_put_grow(0, 42); + outer.at(1).at_put_grow(0, 666); + ASSERT_EQ(outer.at(0).at(0), 42); + ASSERT_EQ(outer.at(1).at(0), 666); + } }; TEST_VM_F(GrowableArrayTest, append) { @@ -460,6 +470,10 @@ TEST_VM_F(GrowableArrayTest, assignment) { with_no_cheap_array_append1(Assignment1); } +TEST_VM_F(GrowableArrayTest, inner_array) { + test_inner_array(); +} + #ifdef ASSERT TEST_VM_F(GrowableArrayTest, where) { WithEmbeddedArray s(1, mtTest); It fails like this: Expected equality of these values: outer.at(0).at(0) Which is: 666 42
28-11-2023

I just learned that [~jsjolen] is working on GrowableArray, see these issues: JDK-8319709 JDK-8314644 JDK-8319117 JDK-8314571 JDK-8319115 Until this is resolved, I would say I cannot really work here.
28-11-2023

[~roland] thanks for fixing it!
15-11-2023

[~epeter] InterfaceSet is fixed now.
14-11-2023

[~roland] I see that you used copy-by-value of "class InterfaceSet" in many places for JDK-8297933. The problem is that "InterfaceSet" is composed of: "GrowableArray<ciKlass*> _list" "ciKlass* _exact_klass" Not sure if the second one could lead to any "use-after-free" issues. But certainly copy-by-value of InterfaceSet implies a copy-by-value of Growable array. I also think it is not ok to just pass around the GrowableArray by reference. That would probably violate the Style guide: one is only supposed to pass references down, not up. Maybe we can use pointers, if we are sure that these pointers do not outlive the GrowableArray's themselves. How is that with the use in "TypePtr", how long do these live? [~roland] Would you mind looking at/removing all the copy-by-value for GrowableArray regarding InterfaceSet? Maybe make InterfaceSet NONCOPYABLE? Feel free to create a subtask and push this BUG back to me afterwards.
30-05-2023

> the methods are actually unused Correction: They are used. I confused myself here.
26-05-2023

After you fix the compiler code that copies GrowableArrays, you can reassign this back to runtime and we'll fix that. Thanks!
17-05-2023

[~epeter] https://github.com/openjdk/jdk/pull/13833 Looks like very good work, which is why we thought of you.
16-05-2023

Thanks, Emanuel!
16-05-2023

[~thartmann][~coleenp][~jvernee][~azafari] I will have a look at it. But only after I get the other one reviewed (other than compiler-team people like [~thartmann] and [~chagedorn]). JDK-8302670 https://github.com/openjdk/jdk/pull/13833
16-05-2023

The code that fails to compile when making GrowableArray NONCOPYABLE is from JDK-8297933 but with below workaround (the methods are actually unused), I see more issues in very old code (also in non-compiler code). --- a/src/hotspot/share/utilities/growableArray.hpp +++ b/src/hotspot/share/utilities/growableArray.hpp @@ -782,6 +782,10 @@ public: this->clear_and_deallocate(); } } + + GrowableArray(GrowableArray&&) = default; + + NONCOPYABLE(GrowableArray); }; --- a/src/hotspot/share/opto/type.hpp +++ b/src/hotspot/share/opto/type.hpp @@ -1298,10 +1298,6 @@ private: ShouldNotReachHere(); return false; } - virtual const InterfaceSet interfaces() const { - return _interfaces; - }; - const TypeOopPtr* is_reference_type(const Type* other) const { return other->isa_oopptr(); } @@ -1716,10 +1712,6 @@ private: ShouldNotReachHere(); return false; } - virtual const InterfaceSet interfaces() const { - return _interfaces; - }; - const TypeKlassPtr* is_reference_type(const Type* other) const { return other->isa_klassptr(); } Another fix: --- a/src/hotspot/share/opto/constantTable.cpp +++ b/src/hotspot/share/opto/constantTable.cpp @@ -291,7 +291,7 @@ ConstantTable::Constant ConstantTable::add_jump_table(MachConstantNode* n) { return con; } -void ConstantTable::fill_jump_table(CodeBuffer& cb, MachConstantNode* n, GrowableArray<Label*> labels) const { +void ConstantTable::fill_jump_table(CodeBuffer& cb, MachConstantNode* n, const GrowableArray<Label*>& labels) const { // If called from Compile::scratch_emit_size do nothing. if (Compile::current()->output()->in_scratch_emit_size()) return; diff --git a/src/hotspot/share/opto/constantTable.hpp b/src/hotspot/share/opto/constantTable.hpp index 001193f6d0d..684d7b76e64 100644 --- a/src/hotspot/share/opto/constantTable.hpp +++ b/src/hotspot/share/opto/constantTable.hpp @@ -172,7 +172,7 @@ public: // Jump-table Constant add_jump_table(MachConstantNode* n); - void fill_jump_table(CodeBuffer& cb, MachConstantNode* n, GrowableArray<Label*> labels) const; + void fill_jump_table(CodeBuffer& cb, MachConstantNode* n, const GrowableArray<Label*>& labels) const; }; ILW = Use-after-free in code using copy constructor for GrowableArray, no issue observed with current code, no workaround = MMH = P3
16-05-2023

It looks like the compiler code uses copies of GrowableArray, so reassigning it to compiler subcomponent.
16-05-2023

After making the GrowableArray as NONCOPYABLE, the following compile error is shown: In file included from src/hotspot/share/opto/node.hpp:30: src/hotspot/share/opto/type.hpp:1258:12: error: call to implicitly-deleted copy constructor of 'const TypePtr::InterfaceSet' return _interfaces; ^~~~~~~~~~~ src/hotspot/share/opto/type.hpp:886:29: note: copy constructor of 'InterfaceSet' is implicitly deleted because field '_list' has a deleted copy constructor GrowableArray<ciKlass*> _list; ^ src/hotspot/share/utilities/growableArray.hpp:680:15: note: 'GrowableArray' has been explicitly marked deleted here NONCOPYABLE(GrowableArray); ^ There are many other case that cause the same compile failure. Maybe, the solution from JDK-8302670 will be applicable here also.
16-05-2023