Delete copy/move on IceRuby classes that register a GC root#5674
Merged
Conversation
RubySliceLoader, ValueWriter and ValueReader register a Ruby GC root tied to the address of a VALUE member (rb_gc_register_address(&_member)), so they are logically non-copyable and non-movable: a default copy/move would not re-register the new instance's address, and the moved-from/copied instance's destructor would unregister an address that was never registered. Mark all four special members deleted. ExceptionReader must stay copyable -- ice_throw does `throw *this` and make_exception_ptr copies it -- and its copy constructor already re-registers a fresh root, so only its move constructor and assignment operators are deleted. Hardening only, no behavior change. Closes #5591
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the IceRuby C++ wrappers around Ruby VALUE handles by explicitly deleting copy/move special members for types that register Ruby GC roots by address (rb_gc_register_address(&member)), preventing accidental object copies/moves that would invalidate GC root registration.
Changes:
- Deleted copy/move constructors and assignment operators for
IceRuby::ValueWriterandIceRuby::ValueReader. - Deleted copy/move constructors and assignment operators for
IceRuby::RubySliceLoader. - Kept
IceRuby::ExceptionReadercopyable (required bythrow *this/make_exception_ptrsemantics) while deleting its move ctor and assignment operators.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ruby/src/IceRuby/Types.h | Deletes copy/move/assign special members for ValueWriter, ValueReader, and partially for ExceptionReader to match GC-root-by-address semantics. |
| ruby/src/IceRuby/RubySliceLoader.h | Deletes copy/move/assign special members for RubySliceLoader to match GC-root-by-address semantics. |
Comment on lines
+511
to
+516
| // Copyable (ice_throw throws *this, and make_exception_ptr copies); the copy constructor re-registers a fresh | ||
| // GC root for _ex. Registering a root tied to the address of _ex makes it non-movable and non-assignable. | ||
| ExceptionReader(const ExceptionReader&); | ||
| ExceptionReader(ExceptionReader&&) = delete; | ||
| ExceptionReader& operator=(const ExceptionReader&) = delete; | ||
| ExceptionReader& operator=(ExceptionReader&&) = delete; |
bernardnormier
approved these changes
Jun 23, 2026
| ExceptionReader(const ExceptionInfoPtr&); | ||
| // Copyable (ice_throw throws *this, and make_exception_ptr copies); the copy constructor re-registers a fresh | ||
| // GC root for _ex. Registering a root tied to the address of _ex makes it non-movable and non-assignable. | ||
| ExceptionReader(const ExceptionReader&); |
Member
There was a problem hiding this comment.
We already have a copy constructor and a destructor, no need to delete the move constructor. I would undo this update to ExceptionReader.
Member
Author
There was a problem hiding this comment.
🤖 Claude: Done in 7cea509 — reverted the ExceptionReader update. The copy constructor + destructor already suppress the implicit move constructor, so the deletes were redundant.
ExceptionReader's user-declared copy constructor and destructor already suppress the implicit move constructor, so deleting the move constructor and the assignment operators was redundant.
pepone
added a commit
that referenced
this pull request
Jul 1, 2026
(cherry picked from commit 3f6d563)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RubySliceLoader,ValueWriterandValueReaderregister a Ruby GC root tied to the address of aVALUEmember (rb_gc_register_address(&_member)), so they are logically non-copyable and non-movable: a default copy/move would not re-register the new instance's address, and the moved-from/copied instance's destructor wouldrb_gc_unregister_addressan address that was never registered. All four special members are now explicitly= deleted.ExceptionReadermust stay copyable —ice_throwdoesthrow *thisandmake_exception_ptrcopies it — and its copy constructor already re-registers a fresh root, so only its move constructor and assignment operators are deleted.This is not an active bug today (these are only ever created with
make_sharedand handled through ashared_ptr), so it's hardening only with no behavior change — a good fit for 3.9.0.Closes #5591