Skip to content

protect module method and function results while wrapping in invoke #1494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kevinushey merged 3 commits into from
Aug 4, 2026
Merged

protect module method and function results while wrapping in invoke #1494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e383a3
protect module method and function results while wrapping in invoke
kevinushey Aug 3, 2026
8a7d8c3
narrow gctorture scope in module regression test
kevinushey Aug 3, 2026
9c05d21
Merge remote-tracking branch 'origin/master' into bugfix/module-invok…
kevinushey Aug 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
2026-08-03 Kevin Ushey <kevinushey@gmail.com>

* inst/include/Rcpp/module/class.h (invoke): Protect freshly
computed method results while wrapping them in the result list
(#1493)
* inst/include/Rcpp/module/Module.h (invoke): Idem for module
function results
* inst/tinytest/cpp/Module.cpp: Add regression test
* inst/tinytest/test_module.R: Idem
2026-08-03 Kevin Ushey <kevinushey@gmail.com>

* inst/include/Rcpp/api/meat/proxy.h: Protect fresh SEXPs returned
Expand Down
3 changes: 2 additions & 1 deletion inst/include/Rcpp/module/Module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ namespace Rcpp {
throw std::range_error( "incorrect number of arguments" ) ;
}

Shield<SEXP> res( fun->operator()( args ) ) ;
return List::create(
_["result"] = fun->operator()( args ),
_["result"] = static_cast<SEXP>(res),
_["void"] = fun->is_void()
) ;
}
Expand Down
3 changes: 2 additions & 1 deletion inst/include/Rcpp/module/class.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@
m->operator()( XP(object), args );
return Rcpp::List::create( true ) ;
} else {
return Rcpp::List::create( false, m->operator()( XP(object), args ) ) ;
Shield<SEXP> res( m->operator()( XP(object), args ) ) ;
return Rcpp::List::create( false, static_cast<SEXP>(res) ) ;

@eddelbuettel eddelbuettel Aug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we cast the result of Shield<SEXP> back to SEXP in other spots?

@kevinushey kevinushey Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one other spot: Environment::assign() has a dedicated Shield<SEXP> overload casting back with (SEXP) (Environment.h:193, added in 8670bb6) -- required there because the templated assign(const WRAPPABLE&) out-matches the SEXP overload for a Shield argument (deduction ignores operator SEXP()) and its implementation would otherwise recurse. Everywhere else Shield feeds plain SEXP-taking C functions, where the implicit conversion applies and no cast is needed.

Strictly, the cast here isn't required: List::create() takes const T& and wrap() has a convertible-to-SEXP fallback, so the uncast form compiles and behaves correctly (verified, including under gctorture) -- Vector::create() itself relies on that path when assigning its Shielded names via res.attr("names"). But with the cast, _["result"] instantiates the named_object<SEXP> specialization rather than a generic named_object<Shield<SEXP>>, keeping this on the standard exact-SEXP paths, so I'd prefer to keep it.

I also checked whether other module-code sites need the same protect-then-cast treatment: invoke_notvoid() and getProperty() return the raw result directly with no allocation afterward, and Module::get_function()'s create() arguments are all rooted (XPtr via the preserve list, formals is a stored Rcpp::List), so these two sites appear to be the only ones with the unprotected window.

}
END_RCPP
}
Expand Down
27 changes: 27 additions & 0 deletions inst/tinytest/cpp/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,27 @@ double Test_get_x_pointer(ModuleTest* x) {
return x->value;
}

class ModuleGadget {
public:
ModuleGadget() {}

// void overload: forces dispatch through class_::invoke(), which
// wraps method results as list(voidness, result)
void value(int x) {
(void) x;
}

// non-void overload: nothing protects the raw SEXP result while
// class_::invoke() allocates the result list
SEXP value() {
SEXP x = Rf_allocVector(REALSXP, 3);
REAL(x)[0] = 1;
REAL(x)[1] = 2;
REAL(x)[2] = 3;
return x;
}
};

RCPP_MODULE(demoModule) {
function("hello", &hello);
function("bar" , &bar );
Expand Down Expand Up @@ -194,6 +215,12 @@ RCPP_MODULE(demoModule) {

.method("get" , &ModuleRandomizer::get)
;

class_<ModuleGadget>("ModuleGadget")
.constructor()
.method("value", static_cast<void (ModuleGadget::*)(int)>(&ModuleGadget::value))
.method("value", static_cast<SEXP (ModuleGadget::*)()>(&ModuleGadget::value))
;
}

// [[Rcpp::export]]
Expand Down
11 changes: 11 additions & 0 deletions inst/tinytest/test_module.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,14 @@ expect_equal(r$get(10), x10)
expect_equal( test_reference( seq(0,10) ), 11L )
expect_equal( test_const_reference( seq(0,10) ), 11L )
expect_equal( test_const( seq(0,10) ), 11L )

## mixed-voidness method overloads dispatch through class_::invoke(),
## which must protect the freshly allocated method result while it
## wraps it in the result list (#1493); under gctorture every
## allocation triggers a collection, so a single call exercises the
## unprotected window deterministically
gadget <- new( ModuleGadget )
gctorture(TRUE)
res <- gadget$value()
gctorture(FALSE)
expect_identical( res, c(1, 2, 3) )
Loading