From 9d605dd807d4682e29e53cd9469ca490b642cf84 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Mon, 3 Aug 2026 05:57:01 -0700 Subject: [PATCH] protect fresh proxy get() results in conversion and assignment operators Proxy operator T() conversions and proxy-to-proxy assignments passed the raw result of get() into allocating calls (as() coercions, set() implementations that allocate before capturing the value). When get() returns a freshly computed SEXP -- an active binding or active RefClass field, a field access evaluated via $, an attribute expanded by Rf_getAttrib() -- that object is referenced from nowhere the garbage collector can see, and a collection triggered inside the window frees it while still in use. Reproducible via gctorture() on an R built with structure checking; see #1491 for details and a reproducible example. Shield the get() result at each such site. Also fix Binding::operator=(const Binding&), whose self-assignment guard ('*this != rhs') failed to compile whenever the operator was actually instantiated; the new regression test is its first instantiation. Fixes #1491. --- ChangeLog | 10 +++++++ inst/include/Rcpp/api/meat/proxy.h | 44 +++++++++++++++++++--------- inst/include/Rcpp/proxy/Binding.h | 8 +++-- inst/include/Rcpp/proxy/NamesProxy.h | 5 +++- inst/include/Rcpp/proxy/SlotProxy.h | 6 ++-- inst/tinytest/cpp/misc.cpp | 10 +++++++ inst/tinytest/test_misc.R | 28 ++++++++++++++++++ 7 files changed, 92 insertions(+), 19 deletions(-) diff --git a/ChangeLog b/ChangeLog index 15cefd8f8..e8595ef78 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2026-08-03 Kevin Ushey + + * inst/include/Rcpp/api/meat/proxy.h: Protect fresh SEXPs returned + from proxy get() in conversion and assignment operators (#1491) + * inst/include/Rcpp/proxy/Binding.h: Idem + * inst/include/Rcpp/proxy/NamesProxy.h: Idem + * inst/include/Rcpp/proxy/SlotProxy.h: Idem + * inst/tinytest/cpp/misc.cpp: Add regression tests + * inst/tinytest/test_misc.R: Idem + 2026-07-24 Dirk Eddelbuettel * DESCRIPTION (Version, Date): Roll micro version and date diff --git a/inst/include/Rcpp/api/meat/proxy.h b/inst/include/Rcpp/api/meat/proxy.h index e7dffde68..7b50a6568 100644 --- a/inst/include/Rcpp/api/meat/proxy.h +++ b/inst/include/Rcpp/api/meat/proxy.h @@ -37,7 +37,8 @@ AttributeProxyPolicy::AttributeProxy::operator=(const T& rhs) { template template AttributeProxyPolicy::AttributeProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template @@ -48,7 +49,8 @@ AttributeProxyPolicy::AttributeProxy::operator SEXP() const { template template AttributeProxyPolicy::const_AttributeProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template @@ -68,13 +70,15 @@ NamesProxyPolicy::NamesProxy::operator=(const T& rhs) { template template NamesProxyPolicy::NamesProxy::operator T() const { - return as( get() ); + Shield x(get()); + return as(x); } template template NamesProxyPolicy::const_NamesProxy::operator T() const { - return as( get() ); + Shield x(get()); + return as(x); } // SlotProxy @@ -89,7 +93,8 @@ SlotProxyPolicy::SlotProxy::operator=(const T& rhs) { template template SlotProxyPolicy::SlotProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } // TagProxy @@ -104,7 +109,8 @@ TagProxyPolicy::TagProxy::operator=(const T& rhs) { template template TagProxyPolicy::TagProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template @@ -115,7 +121,8 @@ TagProxyPolicy::TagProxy::operator SEXP() const { template template TagProxyPolicy::const_TagProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template @@ -135,13 +142,15 @@ BindingPolicy::Binding::operator=(const T& rhs) { template template BindingPolicy::Binding::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template template BindingPolicy::const_Binding::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } // DottedPairProxy @@ -163,20 +172,25 @@ DottedPairProxyPolicy::DottedPairProxy::operator=(const traits::named_obj template template DottedPairProxyPolicy::DottedPairProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template template DottedPairProxyPolicy::const_DottedPairProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } // FieldProxy template typename FieldProxyPolicy::FieldProxy& FieldProxyPolicy::FieldProxy::operator=(const FieldProxyPolicy::FieldProxy& rhs) { - if (this != &rhs) set(rhs.get()); + if (this != &rhs) { + Shield x(rhs.get()); + set(x); + } return *this; } @@ -191,13 +205,15 @@ FieldProxyPolicy::FieldProxy::operator=(const T& rhs) { template template FieldProxyPolicy::FieldProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } template template FieldProxyPolicy::const_FieldProxy::operator T() const { - return as(get()); + Shield x(get()); + return as(x); } } diff --git a/inst/include/Rcpp/proxy/Binding.h b/inst/include/Rcpp/proxy/Binding.h index 3b9d0123d..c56369ecf 100644 --- a/inst/include/Rcpp/proxy/Binding.h +++ b/inst/include/Rcpp/proxy/Binding.h @@ -45,8 +45,12 @@ class BindingPolicy { env.unlockBinding(name) ; } Binding& operator=(const Binding& rhs){ - if( *this != rhs ) - set( rhs.get() ) ; + // NB: '*this != rhs' previously used here would not compile + // when this operator was instantiated + if( env.get__() != rhs.env.get__() || name != rhs.name ) { + Shield x( rhs.get() ) ; + set(x) ; + } return *this ; } diff --git a/inst/include/Rcpp/proxy/NamesProxy.h b/inst/include/Rcpp/proxy/NamesProxy.h index 2717b2a78..d959322b3 100644 --- a/inst/include/Rcpp/proxy/NamesProxy.h +++ b/inst/include/Rcpp/proxy/NamesProxy.h @@ -31,7 +31,10 @@ class NamesProxyPolicy{ /* lvalue uses */ NamesProxy& operator=(const NamesProxy& rhs) { - if( this != &rhs) set( rhs.get() ) ; + if( this != &rhs) { + Shield x( rhs.get() ) ; + set(x) ; + } return *this ; } diff --git a/inst/include/Rcpp/proxy/SlotProxy.h b/inst/include/Rcpp/proxy/SlotProxy.h index c6d3f4f76..df26fc4f1 100644 --- a/inst/include/Rcpp/proxy/SlotProxy.h +++ b/inst/include/Rcpp/proxy/SlotProxy.h @@ -33,7 +33,8 @@ class SlotProxyPolicy { } SlotProxy& operator=(const SlotProxy& rhs){ - set( rhs.get() ) ; + Shield x( rhs.get() ) ; + set(x) ; return *this ; } @@ -65,7 +66,8 @@ class SlotProxyPolicy { } template operator T() const { - return as( get() ); + Shield x( get() ); + return as(x); } inline operator SEXP() const { return get() ; diff --git a/inst/tinytest/cpp/misc.cpp b/inst/tinytest/cpp/misc.cpp index 87271e5d3..d0a4de6f9 100644 --- a/inst/tinytest/cpp/misc.cpp +++ b/inst/tinytest/cpp/misc.cpp @@ -173,6 +173,16 @@ StretchyList named_stretchy_list() { return out; } +// [[Rcpp::export]] +void copy_field_gc(Reference a, Reference b) { + a.field("x") = b.field("y"); +} + +// [[Rcpp::export]] +void copy_binding_gc(Environment a, Environment b) { + a["x"] = b["y"]; +} + // [[Rcpp::export]] void test_stop_variadic() { stop( "%s %d", "foo", 3 ); diff --git a/inst/tinytest/test_misc.R b/inst/tinytest/test_misc.R index 08df4b5cd..3e9730e7a 100644 --- a/inst/tinytest/test_misc.R +++ b/inst/tinytest/test_misc.R @@ -129,6 +129,34 @@ expect_equal(stretchy_list(), pairlist( "foo", 1L, 3.2 )) # test.named_StretchyList <- function(){ expect_equal(named_stretchy_list(), pairlist( a = "foo", b = 1L, c = 3.2 )) +# test.FieldProxy.gc <- function(){ +## copying a field whose value is computed freshly on access must keep +## that value protected across the assignment (#1491) +FooGC <- setRefClass("FooGC", fields = list( + x = "ANY", + y = function(value) { + if (missing(value)) new.env(parent = emptyenv()) else stop("read-only") + } +)) +a <- FooGC$new(x = NULL) +b <- FooGC$new(x = NULL) +gctorture(TRUE) +for (i in 1:20) + copy_field_gc(a, b) +gctorture(FALSE) +expect_true(is.environment(a$x)) + +# test.Binding.gc <- function(){ +## same for environment-to-environment binding copies (#1491) +ea <- new.env() +eb <- new.env() +makeActiveBinding("y", function() new.env(parent = emptyenv()), eb) +gctorture(TRUE) +for (i in 1:20) + copy_binding_gc(ea, eb) +gctorture(FALSE) +expect_true(is.environment(ea$x)) + # test.stop.variadic <- function(){ m <- tryCatch( test_stop_variadic(), error = function(e){ conditionMessage(e)