From 3e2132eaaec3829fc7f359350da19e36cd188397 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Sat, 7 Dec 2024 20:47:09 +0100 Subject: [PATCH 1/5] fix: Avoid premature release for external pointers --- inst/include/cpp11/external_pointer.hpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/inst/include/cpp11/external_pointer.hpp b/inst/include/cpp11/external_pointer.hpp index a62134ec..2f895b51 100644 --- a/inst/include/cpp11/external_pointer.hpp +++ b/inst/include/cpp11/external_pointer.hpp @@ -66,9 +66,15 @@ class external_pointer { data_ = safe[Rf_shallow_duplicate](rhs.data_); } - external_pointer(external_pointer&& rhs) { reset(rhs.release()); } + external_pointer(external_pointer&& rhs) { + data_ = rhs.data_; + rhs.data_ = R_NilValue; + } - external_pointer& operator=(external_pointer&& rhs) noexcept { reset(rhs.release()); } + external_pointer& operator=(external_pointer&& rhs) noexcept { + data_ = rhs.data_; + rhs.data_ = R_NilValue; + } external_pointer& operator=(std::nullptr_t) noexcept { reset(); }; From 51908e89f04cc7d423a51ff6f383fbec69c9b3f5 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 22 Jan 2026 07:05:34 +0100 Subject: [PATCH 2/5] Add test for external_pointer attribute preservation and fix move assignment 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> --- _codeql_detected_source_root | 1 + cpp11test/src/test-external_pointer.cpp | 49 +++++++++++++++++++++++++ inst/include/cpp11/external_pointer.hpp | 1 + 3 files changed, 51 insertions(+) create mode 120000 _codeql_detected_source_root diff --git a/_codeql_detected_source_root b/_codeql_detected_source_root new file mode 120000 index 00000000..945c9b46 --- /dev/null +++ b/_codeql_detected_source_root @@ -0,0 +1 @@ +. \ No newline at end of file diff --git a/cpp11test/src/test-external_pointer.cpp b/cpp11test/src/test-external_pointer.cpp index 897fd4f2..d8b960aa 100644 --- a/cpp11test/src/test-external_pointer.cpp +++ b/cpp11test/src/test-external_pointer.cpp @@ -36,4 +36,53 @@ context("external_pointer-C++") { uniq.reset(); expect_true(deleted == true); } + + test_that("external_pointer preserves attributes when moved (issue #308)") { + // Test move constructor + { + int* value = new int(42); + cpp11::external_pointer p(value); + + // Set an attribute on the external pointer + Rf_setAttrib(p, R_ClassSymbol, Rf_mkString("test_class")); + + // Verify attribute exists before move + SEXP class_attr = Rf_getAttrib(p, R_ClassSymbol); + expect_true(class_attr != R_NilValue); + + // Move the external pointer using move constructor + cpp11::external_pointer p_moved = std::move(p); + + // Verify attribute is preserved after move + SEXP class_attr_after = Rf_getAttrib(p_moved, R_ClassSymbol); + expect_true(class_attr_after != R_NilValue); + expect_true(strcmp(CHAR(STRING_ELT(class_attr_after, 0)), "test_class") == 0); + + // Clean up + delete p_moved.release(); + } + + // Test move assignment operator + { + int* value1 = new int(1); + cpp11::external_pointer p1(value1); + + // Set an attribute on p1 + Rf_setAttrib(p1, R_ClassSymbol, Rf_mkString("test_class")); + + // Create p2 with nullptr (no memory leak) + cpp11::external_pointer p2(nullptr); + + // Move assign p1 to p2 + p2 = std::move(p1); + + // Verify attribute is preserved after move assignment + SEXP class_attr_after = Rf_getAttrib(p2, R_ClassSymbol); + expect_true(class_attr_after != R_NilValue); + expect_true(strcmp(CHAR(STRING_ELT(class_attr_after, 0)), "test_class") == 0); + + // Clean up + delete p2.release(); + } + } } diff --git a/inst/include/cpp11/external_pointer.hpp b/inst/include/cpp11/external_pointer.hpp index 2f895b51..3b4542c9 100644 --- a/inst/include/cpp11/external_pointer.hpp +++ b/inst/include/cpp11/external_pointer.hpp @@ -74,6 +74,7 @@ class external_pointer { external_pointer& operator=(external_pointer&& rhs) noexcept { data_ = rhs.data_; rhs.data_ = R_NilValue; + return *this; } external_pointer& operator=(std::nullptr_t) noexcept { reset(); }; From 6639ef69e018a9a162ca69ab2b7185d0d7629330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 22 Jan 2026 07:06:41 +0100 Subject: [PATCH 3/5] Remove --- _codeql_detected_source_root | 1 - 1 file changed, 1 deletion(-) delete mode 120000 _codeql_detected_source_root diff --git a/_codeql_detected_source_root b/_codeql_detected_source_root deleted file mode 120000 index 945c9b46..00000000 --- a/_codeql_detected_source_root +++ /dev/null @@ -1 +0,0 @@ -. \ No newline at end of file From 77060f5ee96611f59b0343a9f15088835d2f2acc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 22 Jan 2026 07:35:10 +0100 Subject: [PATCH 4/5] Clarify --- inst/include/cpp11/external_pointer.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/inst/include/cpp11/external_pointer.hpp b/inst/include/cpp11/external_pointer.hpp index 3b4542c9..2957b01f 100644 --- a/inst/include/cpp11/external_pointer.hpp +++ b/inst/include/cpp11/external_pointer.hpp @@ -72,7 +72,9 @@ class external_pointer { } external_pointer& operator=(external_pointer&& rhs) noexcept { + // This works even if `this == &rhs` because `data_` (a `cpp11::sexp`) handles the underlying resource. data_ = rhs.data_; + // Order matters: first assign, then clear the RHS. rhs.data_ = R_NilValue; return *this; } From 56041ba36c45020bf83c108e228225d34253d347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kirill=20M=C3=BCller?= Date: Thu, 22 Jan 2026 07:37:45 +0100 Subject: [PATCH 5/5] Formatting --- inst/include/cpp11/external_pointer.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/inst/include/cpp11/external_pointer.hpp b/inst/include/cpp11/external_pointer.hpp index 2957b01f..04d8fb4a 100644 --- a/inst/include/cpp11/external_pointer.hpp +++ b/inst/include/cpp11/external_pointer.hpp @@ -72,7 +72,8 @@ class external_pointer { } external_pointer& operator=(external_pointer&& rhs) noexcept { - // This works even if `this == &rhs` because `data_` (a `cpp11::sexp`) handles the underlying resource. + // This works even if `this == &rhs` because `data_` (a `cpp11::sexp`) handles + // the underlying resource. data_ = rhs.data_; // Order matters: first assign, then clear the RHS. rhs.data_ = R_NilValue;