RFR: 8302670: use-after-free related to PhaseIterGVN interaction with Unique_Node_List and Node_Stack [v10]
Emanuel Peter
epeter at openjdk.org
Mon May 22 12:35:56 UTC 2023
On Fri, 19 May 2023 20:10:27 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
>>> > OTOH, I think the rationale of needing a move constructor to permit returning noncopyable objects from functions is eliminated by C++17's guaranteed copy elision.
>>>
>>> @kimbarrett @jcking I wonder if it is not better to just avoid any move constructors/assign. We would have to convert the "return value optimization" cases (i.e. return contrainer by value, and it being captured by the move constructor), and instead create containers outside, and pass them in as references ("pseudo-output"). It is a bit ugly, but maybe more understandable than the move semantics?
>>>
>>> Once we'd take on C++17, we can still reconsider changing patterns and returning containers with the guaranteed copy elision.
>>
>> I'm not generally much of a fan of out-ref parameters.
>>
>> I see the move constructors in this PR as being workarounds for our _current_
>> lack of C++17 guaranteed copy elision. So I would be okay with keeping them,
>> with an RFE to remove them once they are no longer needed (i.e. we're using
>> C++17 or later). Label that RFE with `cpp17`. (That's a new label; we have
>> `cpp14` and `cpp20` but nothing with `cpp17` yet).
>>
>> Ignore my comment about move-assign operators for now, and don't bother with
>> them unless and until actually needed (which might be never). There's no
>> deprecation warning issue related to not having them.
>
>> @kimbarrett @jcking So if we never use `std::move` (currently not used at all in the HotSpot code), do I actually need to have a custom implementation of the move-constructor, or is the `default` enough?
>
> It depends on the class whether a simple shallow-copy (the default) is an
> adequate move. The main question is whether the destruction of the moved-from
> object might damage/delete something that was pilfered for use in the moved-to
> object. Since from a quick skim it looked like the destructors for all of the
> classes being given move constructors are empty or (implicitly) defaulted, I
> think that shouldn’t be a problem. But someone should check them more
> carefully. In particular, are there any bases or nested members with
> non-trivial destructors?
>
> The problem with std::move is that one can convert an lvalue to an rvalue,
> move construct from the rvalue, and then try to use both the old lvalue
> (potential UB) and the newly moved-to object, even though they share state
> (source of UB). (Also remember that std::move is basically just a convenience
> wrapper over a cast of an lvalue to an rvalue - there’s nothing magic about
> it. But we’re not doing that anywhere either.)
This is what I have so far, using `std::move` explicitly. @kimbarrett @jcking I'm not sure this is very nice. Any suggestions?
diff --git a/src/hotspot/share/libadt/dict.hpp b/src/hotspot/share/libadt/dict.hpp
index c021536c402..0a9ff85c192 100644
--- a/src/hotspot/share/libadt/dict.hpp
+++ b/src/hotspot/share/libadt/dict.hpp
@@ -62,7 +62,20 @@ class Dict : public AnyObj { // Dictionary structure
~Dict();
// Allow move constructor for && (eg. capture return of function)
- Dict(Dict&&) = default;
+ Dict(Dict&& other) : _arena(other._arena),
+ _bin(other._bin),
+ _size(other._size),
+ _cnt(other._cnt),
+ _hash(other._hash),
+ _cmp(other._cmp) {
+ // Other is invalidated: SIGSEGV upon use.
+ other._arena = nullptr;
+ other._bin = nullptr;
+ other._size = 0;
+ other._cnt = 0;
+ };
+ Dict& operator=(Dict&&) = delete;
+ NONCOPYABLE(Dict);
// Return # of key-value pairs in dict
uint32_t Size(void) const { return _cnt; }
@@ -78,8 +91,6 @@ class Dict : public AnyObj { // Dictionary structure
// Print out the dictionary contents as key-value pairs
void print();
-
- NONCOPYABLE(Dict);
};
// Hashing functions
diff --git a/src/hotspot/share/libadt/vectset.hpp b/src/hotspot/share/libadt/vectset.hpp
index a82046f2ba9..c7f08da9e43 100644
--- a/src/hotspot/share/libadt/vectset.hpp
+++ b/src/hotspot/share/libadt/vectset.hpp
@@ -25,6 +25,7 @@
#ifndef SHARE_LIBADT_VECTSET_HPP
#define SHARE_LIBADT_VECTSET_HPP
+#include <utility>
#include "memory/allocation.hpp"
#include "utilities/copy.hpp"
@@ -56,7 +57,18 @@ public:
~VectorSet() {}
// Allow move constructor for && (eg. capture return of function)
- VectorSet(VectorSet&&) = default;
+ VectorSet(VectorSet&& other) : _size(other._size),
+ _data(other._data),
+ _data_size(other._data_size),
+ _set_arena(other._set_arena) {
+ // Other is invalidated: reads as if empty, SIGSEGV upon write.
+ other._size = 0;
+ other._data = nullptr;
+ other._data_size = 0;
+ other._set_arena = nullptr;
+ };
+ VectorSet& operator=(VectorSet&&) = delete;
+ NONCOPYABLE(VectorSet);
void insert(uint elem);
bool is_empty() const;
@@ -113,8 +125,6 @@ public:
uint32_t mask = 1U << (elem & bit_mask);
_data[word] |= mask;
}
-
- NONCOPYABLE(VectorSet);
};
#endif // SHARE_LIBADT_VECTSET_HPP
diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp
index 536a4e7b46d..3f93406e18f 100644
--- a/src/hotspot/share/opto/compile.cpp
+++ b/src/hotspot/share/opto/compile.cpp
@@ -2192,6 +2192,60 @@ void Compile::remove_root_to_sfpts_edges(PhaseIterGVN& igvn) {
void Compile::Optimize() {
TracePhase tp("optimizer", &timers[_t_optimizer]);
+ // TODO remove playground
+ VectorSet sss;
+ sss.insert(10000);
+ tty->print_cr("VectorSet 1: %d", sss.test(10000));
+ VectorSet s2(std::move(sss));
+ tty->print_cr("VectorSet 2: %d", s2.test(10000));
+ tty->print_cr("VectorSet 3: %d", sss.test(10000));
+ s2.insert(20000);
+ tty->print_cr("VectorSet 4: %d", s2.test(20000));
+ // SIGSEGV upon insert:
+ // sss.insert(10000);
+
+ Dict d1(cmpkey, hashkey);
+ d1.Insert((void*)1000, (void*)42);
+ tty->print_cr("Dict 1: %ld", (long)d1[(void*)1000]);
+ Dict d2(std::move(d1));
+ tty->print_cr("Dict 2: %ld", (long)d2[(void*)1000]);
+ // SIGSEGV upon read:
+ //tty->print_cr("Dict 3: %ld", (long)d1[(void*)1000]);
+
+ Node_Array a1;
+ a1.map(10000, (Node*)42);
+ tty->print_cr("Node_Array 1: %ld", (long)a1.at(10000));
+ Node_Array a2(std::move(a1));
+ tty->print_cr("Node_Array 2: %ld", (long)a2.at(10000));
+ tty->print_cr("Node_Array 3: max %ld", (long)a1.max());
+ // oob assert on read:
+ // tty->print_cr("Node_Array 4: %ld", (long)a1.at(10000));
+ // asserts because _max not larger than zero:
+ // a1.map(10000, (Node*)666);
+
+ Node_List l1;
+ l1.push((Node*)42);
+ tty->print_cr("Node_List 1: %ld", (long)l1.at(0));
+ Node_List l2(std::move(l1));
+ tty->print_cr("Node_List 2: %ld", (long)l2.at(0));
+ tty->print_cr("Node_List 3: size %ld", (long)l1.size());
+ // asserts because _max not larger than zero:
+ // l1.push((Node*)666);
+
+ Unique_Node_List w1;
+ Node* nt0 = new Node(2);
+ w1.push(nt0);
+ w1.dump();
+ Node_List w2(std::move(w1));
+ w2.dump();
+ tty->print_cr("Unique_Node_List 1: size %ld", (long)w1.size());
+ w1.dump();
+ // does not quite behave correctly when write to w1
+ Node* nt1 = new Node(2);
+ w1.push(nt0);
+ tty->print_cr("Unique_Node_List 2: size %ld", (long)w1.size());
+ w1.dump();
+
#ifndef PRODUCT
if (env()->break_at_compile()) {
BREAKPOINT;
diff --git a/src/hotspot/share/opto/node.hpp b/src/hotspot/share/opto/node.hpp
index 43375187abc..e65915acbba 100644
--- a/src/hotspot/share/opto/node.hpp
+++ b/src/hotspot/share/opto/node.hpp
@@ -1540,7 +1540,16 @@ public:
Node_Array() : Node_Array(Thread::current()->resource_area()) {}
// Allow move constructor for && (eg. capture return of function)
- Node_Array(Node_Array&&) = default;
+ Node_Array(Node_Array&& other) : _a(other._a),
+ _max(other._max),
+ _nodes(other._nodes) {
+ // Other is invalidated: reads as if empty, asserts on write.
+ other._a = nullptr;
+ other._max = 0;
+ other._nodes = nullptr;
+ };
+ Node_Array& operator=(Node_Array&&) = delete;
+ NONCOPYABLE(Node_Array);
Node *operator[] ( uint i ) const // Lookup, or null for not mapped
{ return (i<_max) ? _nodes[i] : (Node*)nullptr; }
@@ -1557,8 +1566,6 @@ public:
uint max() const { return _max; }
void dump() const;
-
- NONCOPYABLE(Node_Array);
};
class Node_List : public Node_Array {
@@ -1569,7 +1576,12 @@ public:
Node_List(Arena *a, uint max = OptoNodeListSize) : Node_Array(a, max), _cnt(0) {}
// Allow move constructor for && (eg. capture return of function)
- Node_List(Node_List&&) = default;
+ Node_List(Node_List&& other) : Node_Array(std::move(other)), _cnt(other._cnt) {
+ // Other is invalidated: reads as if empty, asserts on write.
+ other._cnt = 0;
+ };
+ Node_List& operator=(Node_List&&) = delete;
+ NONCOPYABLE(Node_List);
bool contains(const Node* n) const {
for (uint e = 0; e < size(); e++) {
@@ -1594,8 +1606,6 @@ public:
uint size() const { return _cnt; }
void dump() const;
void dump_simple() const;
-
- NONCOPYABLE(Node_List);
};
//------------------------------Unique_Node_List-------------------------------
@@ -1608,7 +1618,12 @@ public:
Unique_Node_List(Arena *a) : Node_List(a), _in_worklist(a), _clock_index(0) {}
// Allow move constructor for && (eg. capture return of function)
- Unique_Node_List(Unique_Node_List&&) = default;
+ Unique_Node_List(Unique_Node_List&& other) : Node_List(std::move(other)),
+ _in_worklist(std::move(other._in_worklist)),
+ _clock_index(std::move(other._clock_index)) {
+ // Other is invalidated: reads as if empty, may behave incorrectly on write.
+ other._clock_index = 0;
+ };
void remove( Node *n );
bool member( Node *n ) { return _in_worklist.test(n->_idx) != 0; }
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13833#issuecomment-1557140244
More information about the hotspot-dev
mailing list