Skip to content

Commit 9d605dd

Browse files
kevinusheykevinushey
committed
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<T>() 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.
1 parent 3a9aeaa commit 9d605dd

7 files changed

Lines changed: 92 additions & 19 deletions

File tree

‎ChangeLog‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
2026-08-03 Kevin Ushey <kevinushey@gmail.com>
2+
3+
* inst/include/Rcpp/api/meat/proxy.h: Protect fresh SEXPs returned
4+
from proxy get() in conversion and assignment operators (#1491)
5+
* inst/include/Rcpp/proxy/Binding.h: Idem
6+
* inst/include/Rcpp/proxy/NamesProxy.h: Idem
7+
* inst/include/Rcpp/proxy/SlotProxy.h: Idem
8+
* inst/tinytest/cpp/misc.cpp: Add regression tests
9+
* inst/tinytest/test_misc.R: Idem
10+
111
2026-07-24 Dirk Eddelbuettel <edd@debian.org>
212

313
* DESCRIPTION (Version, Date): Roll micro version and date

‎inst/include/Rcpp/api/meat/proxy.h‎

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ AttributeProxyPolicy<CLASS>::AttributeProxy::operator=(const T& rhs) {
3737
template <typename CLASS>
3838
template <typename T>
3939
AttributeProxyPolicy<CLASS>::AttributeProxy::operator T() const {
40-
return as<T>(get());
40+
Shield<SEXP> x(get());
41+
return as<T>(x);
4142
}
4243

4344
template <typename CLASS>
@@ -48,7 +49,8 @@ AttributeProxyPolicy<CLASS>::AttributeProxy::operator SEXP() const {
4849
template <typename CLASS>
4950
template <typename T>
5051
AttributeProxyPolicy<CLASS>::const_AttributeProxy::operator T() const {
51-
return as<T>(get());
52+
Shield<SEXP> x(get());
53+
return as<T>(x);
5254
}
5355

5456
template <typename CLASS>
@@ -68,13 +70,15 @@ NamesProxyPolicy<CLASS>::NamesProxy::operator=(const T& rhs) {
6870
template <typename CLASS>
6971
template <typename T>
7072
NamesProxyPolicy<CLASS>::NamesProxy::operator T() const {
71-
return as<T>( get() );
73+
Shield<SEXP> x(get());
74+
return as<T>(x);
7275
}
7376

7477
template <typename CLASS>
7578
template <typename T>
7679
NamesProxyPolicy<CLASS>::const_NamesProxy::operator T() const {
77-
return as<T>( get() );
80+
Shield<SEXP> x(get());
81+
return as<T>(x);
7882
}
7983

8084
// SlotProxy
@@ -89,7 +93,8 @@ SlotProxyPolicy<CLASS>::SlotProxy::operator=(const T& rhs) {
8993
template <typename CLASS>
9094
template <typename T>
9195
SlotProxyPolicy<CLASS>::SlotProxy::operator T() const {
92-
return as<T>(get());
96+
Shield<SEXP> x(get());
97+
return as<T>(x);
9398
}
9499

95100
// TagProxy
@@ -104,7 +109,8 @@ TagProxyPolicy<CLASS>::TagProxy::operator=(const T& rhs) {
104109
template <typename CLASS>
105110
template <typename T>
106111
TagProxyPolicy<CLASS>::TagProxy::operator T() const {
107-
return as<T>(get());
112+
Shield<SEXP> x(get());
113+
return as<T>(x);
108114
}
109115

110116
template <typename CLASS>
@@ -115,7 +121,8 @@ TagProxyPolicy<CLASS>::TagProxy::operator SEXP() const {
115121
template <typename CLASS>
116122
template <typename T>
117123
TagProxyPolicy<CLASS>::const_TagProxy::operator T() const {
118-
return as<T>(get());
124+
Shield<SEXP> x(get());
125+
return as<T>(x);
119126
}
120127

121128
template <typename CLASS>
@@ -135,13 +142,15 @@ BindingPolicy<CLASS>::Binding::operator=(const T& rhs) {
135142
template <typename CLASS>
136143
template <typename T>
137144
BindingPolicy<CLASS>::Binding::operator T() const {
138-
return as<T>(get());
145+
Shield<SEXP> x(get());
146+
return as<T>(x);
139147
}
140148

141149
template <typename CLASS>
142150
template <typename T>
143151
BindingPolicy<CLASS>::const_Binding::operator T() const {
144-
return as<T>(get());
152+
Shield<SEXP> x(get());
153+
return as<T>(x);
145154
}
146155

147156
// DottedPairProxy
@@ -163,20 +172,25 @@ DottedPairProxyPolicy<CLASS>::DottedPairProxy::operator=(const traits::named_obj
163172
template <typename CLASS>
164173
template <typename T>
165174
DottedPairProxyPolicy<CLASS>::DottedPairProxy::operator T() const {
166-
return as<T>(get());
175+
Shield<SEXP> x(get());
176+
return as<T>(x);
167177
}
168178

169179
template <typename CLASS>
170180
template <typename T>
171181
DottedPairProxyPolicy<CLASS>::const_DottedPairProxy::operator T() const {
172-
return as<T>(get());
182+
Shield<SEXP> x(get());
183+
return as<T>(x);
173184
}
174185

175186
// FieldProxy
176187
template <typename CLASS>
177188
typename FieldProxyPolicy<CLASS>::FieldProxy&
178189
FieldProxyPolicy<CLASS>::FieldProxy::operator=(const FieldProxyPolicy<CLASS>::FieldProxy& rhs) {
179-
if (this != &rhs) set(rhs.get());
190+
if (this != &rhs) {
191+
Shield<SEXP> x(rhs.get());
192+
set(x);
193+
}
180194
return *this;
181195
}
182196

@@ -191,13 +205,15 @@ FieldProxyPolicy<CLASS>::FieldProxy::operator=(const T& rhs) {
191205
template <typename CLASS>
192206
template <typename T>
193207
FieldProxyPolicy<CLASS>::FieldProxy::operator T() const {
194-
return as<T>(get());
208+
Shield<SEXP> x(get());
209+
return as<T>(x);
195210
}
196211

197212
template <typename CLASS>
198213
template <typename T>
199214
FieldProxyPolicy<CLASS>::const_FieldProxy::operator T() const {
200-
return as<T>(get());
215+
Shield<SEXP> x(get());
216+
return as<T>(x);
201217
}
202218

203219
}

‎inst/include/Rcpp/proxy/Binding.h‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,12 @@ class BindingPolicy {
4545
env.unlockBinding(name) ;
4646
}
4747
Binding& operator=(const Binding& rhs){
48-
if( *this != rhs )
49-
set( rhs.get() ) ;
48+
// NB: '*this != rhs' previously used here would not compile
49+
// when this operator was instantiated
50+
if( env.get__() != rhs.env.get__() || name != rhs.name ) {
51+
Shield<SEXP> x( rhs.get() ) ;
52+
set(x) ;
53+
}
5054
return *this ;
5155
}
5256

‎inst/include/Rcpp/proxy/NamesProxy.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ class NamesProxyPolicy{
3131

3232
/* lvalue uses */
3333
NamesProxy& operator=(const NamesProxy& rhs) {
34-
if( this != &rhs) set( rhs.get() ) ;
34+
if( this != &rhs) {
35+
Shield<SEXP> x( rhs.get() ) ;
36+
set(x) ;
37+
}
3538
return *this ;
3639
}
3740

‎inst/include/Rcpp/proxy/SlotProxy.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ class SlotProxyPolicy {
3333
}
3434

3535
SlotProxy& operator=(const SlotProxy& rhs){
36-
set( rhs.get() ) ;
36+
Shield<SEXP> x( rhs.get() ) ;
37+
set(x) ;
3738
return *this ;
3839
}
3940

@@ -65,7 +66,8 @@ class SlotProxyPolicy {
6566
}
6667

6768
template <typename T> operator T() const {
68-
return as<T>( get() );
69+
Shield<SEXP> x( get() );
70+
return as<T>(x);
6971
}
7072
inline operator SEXP() const {
7173
return get() ;

‎inst/tinytest/cpp/misc.cpp‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,16 @@ StretchyList named_stretchy_list() {
173173
return out;
174174
}
175175

176+
// [[Rcpp::export]]
177+
void copy_field_gc(Reference a, Reference b) {
178+
a.field("x") = b.field("y");
179+
}
180+
181+
// [[Rcpp::export]]
182+
void copy_binding_gc(Environment a, Environment b) {
183+
a["x"] = b["y"];
184+
}
185+
176186
// [[Rcpp::export]]
177187
void test_stop_variadic() {
178188
stop( "%s %d", "foo", 3 );

‎inst/tinytest/test_misc.R‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,34 @@ expect_equal(stretchy_list(), pairlist( "foo", 1L, 3.2 ))
129129
# test.named_StretchyList <- function(){
130130
expect_equal(named_stretchy_list(), pairlist( a = "foo", b = 1L, c = 3.2 ))
131131

132+
# test.FieldProxy.gc <- function(){
133+
## copying a field whose value is computed freshly on access must keep
134+
## that value protected across the assignment (#1491)
135+
FooGC <- setRefClass("FooGC", fields = list(
136+
x = "ANY",
137+
y = function(value) {
138+
if (missing(value)) new.env(parent = emptyenv()) else stop("read-only")
139+
}
140+
))
141+
a <- FooGC$new(x = NULL)
142+
b <- FooGC$new(x = NULL)
143+
gctorture(TRUE)
144+
for (i in 1:20)
145+
copy_field_gc(a, b)
146+
gctorture(FALSE)
147+
expect_true(is.environment(a$x))
148+
149+
# test.Binding.gc <- function(){
150+
## same for environment-to-environment binding copies (#1491)
151+
ea <- new.env()
152+
eb <- new.env()
153+
makeActiveBinding("y", function() new.env(parent = emptyenv()), eb)
154+
gctorture(TRUE)
155+
for (i in 1:20)
156+
copy_binding_gc(ea, eb)
157+
gctorture(FALSE)
158+
expect_true(is.environment(ea$x))
159+
132160
# test.stop.variadic <- function(){
133161
m <- tryCatch( test_stop_variadic(), error = function(e){
134162
conditionMessage(e)

0 commit comments

Comments
 (0)