Fix GC root leak in RefPtr assignment operators#2699
Conversation
The copy and move assignment operators for RefPtr (both same-type and cross-type) were overwriting root_index_ with a new root without first calling DeleteRoot on the old one. This violates the RAII invariant that the destructor's reset() path is supposed to maintain: every CopyRoot must be balanced by a DeleteRoot. In a long-running interpreter session where RefPtr objects are reassigned frequently, the leaked roots accumulate in the Store's root list. Because roots prevent the GC from collecting the referenced objects, this leads to unbounded memory growth. Fix by calling reset() at the top of each assignment operator so the previous root is released before a new one is acquired. The same-type operators also gain a self-assignment guard to avoid use-after-reset.
zherczeg
left a comment
There was a problem hiding this comment.
Yes, it looks like a serious bug.
| template <typename T> | ||
| template <typename U> | ||
| RefPtr<T>& RefPtr<T>::operator=(const RefPtr<U>& other) { | ||
| reset(); |
There was a problem hiding this comment.
Why if (this == &other) return *this; is not added here?
I suspect the point of this check, is that if you run a reset on yourself, then you copy null values later (set by the reset), and just loose the object forever.
There was a problem hiding this comment.
Sorry, I see in your description that this case cannot happen. I think an assert would be still useful here that it really never happens.
There was a problem hiding this comment.
Good point. Added an assert to both cross-type assignment operators to document the invariant. Since T and U are different types self-assignment can't happen through normal use, but the assert catches any unexpected aliasing.
While cross-type assignment (RefPtr<T> from RefPtr<U>) cannot self-assign because T and U are different types, add a defensive assert to document this invariant and catch any unexpected aliasing through type erasure.
Summary
The copy and move assignment operators for
RefPtr(both same-type and cross-type) overwriteroot_index_with a new root without first callingDeleteRooton the old one. While the destructor correctly releases the root viareset(), assignments bypass this cleanup, leaking one GC root per reassignment.In a long-running interpreter session where
RefPtrobjects are reassigned frequently, the leaked roots accumulate in theStore's root list. Because roots prevent the GC from collecting the referenced objects, this leads to unbounded memory growth.Changes
reset()at the top of each assignment operator so the previous root is released before a new one is acquired.if (this == &other) return *this;) to the same-type copy and move assignment operators to avoid use-after-reset on self-assignment.The cross-type assignment operators (
RefPtr<T>::operator=(const RefPtr<U>&)andRefPtr<T>::operator=(RefPtr<U>&&)) do not need a self-assignment guard becauseTandUare different types, sothisand&othercannot alias.Test plan
wasm-interp(which exercisesRefPtrheavily through the interpreter) builds cleanly with no warnings.RefPtrusage paths.