fix: Avoid premature release for external pointers #423
Conversation
r-lib/cpp11#423 This reverts commit 1b698e533ea7c7003cb610a5f5f7e5a47966c59d.
* Revert "ifndef" This reverts commit 9afb676. * Revert "Reapply 782630b minus the whitespace changes" This reverts commit f05d56c. * Reapply "Revert a011d42" This reverts commit fe615c4. * Reapply "Revert fb18a2d" This reverts commit 6f4979d. * Vendor cpp11 0.5.1 * Add `prot` argument to `external_pointer()` constructor r-lib/cpp11#420 This reverts commit abcae40e2aff8b1664098294959b8df743605737. * END_CPP11_EX() r-lib/cpp11#421 * Protection r-lib/cpp11#422 This reverts commit 1e181e5e5c2cf0a21906b49dafe80364c8ca3f2b. * External pointer premature release r-lib/cpp11#423 This reverts commit 1b698e533ea7c7003cb610a5f5f7e5a47966c59d.
* Revert "ifndef" This reverts commit 9afb676. * Revert "Reapply 782630b minus the whitespace changes" This reverts commit f05d56c. * Reapply "Revert a011d42" This reverts commit fe615c4. * Reapply "Revert fb18a2d" This reverts commit 6f4979d. * Vendor cpp11 0.5.1 * Add `prot` argument to `external_pointer()` constructor r-lib/cpp11#420 This reverts commit abcae40e2aff8b1664098294959b8df743605737. * END_CPP11_EX() r-lib/cpp11#421 * Protection r-lib/cpp11#422 This reverts commit 1e181e5e5c2cf0a21906b49dafe80364c8ca3f2b. * External pointer premature release r-lib/cpp11#423 This reverts commit 1b698e533ea7c7003cb610a5f5f7e5a47966c59d.
* Revert "ifndef" This reverts commit 9afb676. * Revert "Reapply 782630b minus the whitespace changes" This reverts commit f05d56c. * Reapply "Revert a011d42" This reverts commit fe615c4. * Reapply "Revert fb18a2d" This reverts commit 6f4979d. * Vendor cpp11 0.5.1 * Add `prot` argument to `external_pointer()` constructor r-lib/cpp11#420 This reverts commit abcae40e2aff8b1664098294959b8df743605737. * END_CPP11_EX() r-lib/cpp11#421 * Protection r-lib/cpp11#422 This reverts commit 1e181e5e5c2cf0a21906b49dafe80364c8ca3f2b. * External pointer premature release r-lib/cpp11#423 This reverts commit 1b698e533ea7c7003cb610a5f5f7e5a47966c59d.
I found the same while porting magick |
r-lib/cpp11#423 This reverts commit 1b698e533ea7c7003cb610a5f5f7e5a47966c59d.
* Revert "External pointer premature release" This reverts commit d7886e3. * Revert "Protection" This reverts commit 4f4a3a8. * Revert "END_CPP11_EX()" This reverts commit aa3225d. * Revert "Add `prot` argument to `external_pointer()` constructor" This reverts commit d045ccf. * Header * Substance * Add `prot` argument to `external_pointer()` constructor r-lib/cpp11#420 This reverts commit abcae40e2aff8b1664098294959b8df743605737. * END_CPP11_EX() r-lib/cpp11#421 * Protection r-lib/cpp11#422 This reverts commit 1e181e5e5c2cf0a21906b49dafe80364c8ca3f2b. * External pointer premature release r-lib/cpp11#423 This reverts commit 1b698e533ea7c7003cb610a5f5f7e5a47966c59d.
|
I imagine that this would need a test. Copilot does not talk to me today, but I remember testing with the motivating example in #308, and the vendored and patched version of cpp11 works just fine for me. |
hi! |
and you can vendor to src, I tagged you here pachadotdev/cpp4r@11afe2f |
…ignment operator (#486) Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the external_pointer class where move operations were causing premature release of external pointers, potentially leading to data corruption or crashes. The fix ensures that attributes attached to external pointers are preserved during move operations.
Changes:
- Modified move constructor and move assignment operator to transfer ownership without calling
reset()andrelease() - Added comprehensive test coverage for verifying attribute preservation during move operations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| inst/include/cpp11/external_pointer.hpp | Fixed move constructor and move assignment operator to preserve pointer attributes by directly transferring data_ instead of using reset(release()) |
| cpp11test/src/test-external_pointer.cpp | Added test cases to verify that external pointer attributes are preserved during move constructor and move assignment operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| external_pointer& operator=(external_pointer&& rhs) noexcept { | ||
| data_ = rhs.data_; | ||
| rhs.data_ = R_NilValue; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
The move assignment operator does not handle self-assignment correctly. If this == &rhs, the operation will set data_ to R_NilValue before returning, resulting in a null pointer. Add a self-assignment check at the beginning of the function.
| data_ = rhs.data_; | ||
| rhs.data_ = R_NilValue; |
There was a problem hiding this comment.
The move assignment operator overwrites data_ without releasing the previously held resource, potentially causing a memory leak. The old data_ should be released before assigning the new value.
| data_ = rhs.data_; | |
| rhs.data_ = R_NilValue; | |
| if (this != &rhs) { | |
| if (data_ != R_NilValue) { | |
| r_deleter(data_); | |
| } | |
| data_ = rhs.data_; | |
| rhs.data_ = R_NilValue; | |
| } |
There was a problem hiding this comment.
This is wrong and defeats the purpose of this PR. Added a clarification comment.
|
Still to be done: run tests with existing cpp11 code to see if they fail. |
From duckdb, see #401.
Fixes #308.
This is fairly severe.