Repository navigation
validate module objects before converting them to c++ classes - #1524
Conversation
eddelbuettel
left a comment
There was a problem hiding this comment.
I think this one may warrant a rev.dep run on its own.
|
I have rebased this on master (can also force-push this if you want) and will reverse-depends check it. |
|
Reverse dependencies look generally good but we have one (1) new regression with a package that passes with the CRAN version: RDelta. A new package, if we think the change is at his end we can probably get the maintainer to adjust. In fact, it does go away with one additional diff -ru RDelta.orig/src/rdelta_wrapper.cpp RDelta/src/rdelta_wrapper.cpp
--- RDelta.orig/src/rdelta_wrapper.cpp 2026-09-04 19:33:50.000000000 +0000
+++ RDelta/src/rdelta_wrapper.cpp 2026-11-03 12:12:54.699757662 +0000
@@ -662,7 +662,7 @@
if (deps.size() > 0) {
CharacterVector key(1);
key[0] = std::to_string(ccnum) + "," + std::to_string(ccstate);
- result[key] = deps;
+ result[key] = Rcpp::wrap(deps);
}
}
}I'll reach out. |
17b8bc6 to
f4b5c39
Compare
|
And I force-pushed, so we can wait for test to (re-)run but you can merge this now methinks. |
eddelbuettel
left a comment
There was a problem hiding this comment.
LGTM and (near perfectly) passes a reverse-dependency test
|
PS By now filed a short and simple PR for |
Fixes #1523.
Converting an R object to a C++ class exposed by a module (
as<T>,as<T&>,as<const T&>,as<T*>,as<const T*>, so any module function or method parameter of such a type, and// [[Rcpp::export]]functions taking one) returned the object's.pointeraddress without any checks. Objects of the wrong class were reinterpreted (silent memory corruption), and deserialized objects handed C++ a NULL pointer (segfault). See #1523 for the reprex.Changes:
inst/include/Rcpp/api/meat/as.h:as_module_object_internal(SEXP)now checks that the object is a live module object: it raisesExpecting a C++ module objectfor non-module arguments,C++ object not initialized. (Missing default constructor?)for the dummy object of a class without a default constructor, andexternal pointer is not validfor a NULL pointer, e.g. aftersaveRDS()/readRDS(). The last two match the errors a method call on such an object already gives. This overload is kept because packages call it directly (e.g. MPCR).as_module_object_internal(SEXP, const char* typeinfo_name)additionally checks the object's class (via.cppclass): it must be the requested C++ type, or a class that derives from it throughclass_<>::derives<T>()(parents are resolved through the object's module, so multi-level hierarchies work). Otherwise:Expecting an object of C++ class 'Point', got an object of class 'Label'.inst/include/Rcpp/as.h: the module-objectas<>dispatch passestypeid(T).name()to the new overload.Everything is header code compiled into the calling package; nothing in
Rcpp.soor in the layout of the module classes changes.Behavior change to be aware of: passing an object of an unrelated module class is now an error. That includes a C++ subclass that is exposed without
.derives<Base>()and passed where aBase&is expected. That case only worked by accident (the base subobject happened to sit at offset 0), but code relying on it would now need a.derives<Base>("Base"), so this warrants a reverse-dependency check.Tests (
inst/tinytest/test_module.R, with aModuleBase/ModuleDerivedpair added tocpp/Module.cpp):// [[Rcpp::export]]functionderives()from the expected class is acceptedVerification:
The reprex from module object arguments are converted without any validity or class checks #1523 segfaults against current master. On this branch every call errors cleanly instead (and
label$textis left intact):Against master the new tests segfault; on this branch
test_module.R(52/52),test_modref.R,test_expose_class.R,test_module_client_package.R,test_attributes.R,test_rcpp_package_skeleton.R,test_wrap.Randtest_as.Rpass withRunAllRcppTests=yes RunVerboseRcppTests=yes(R 4.6.1, macOS).test_module.Rstill printsError: Expecting an external pointer: [type=environment].at exit for its uninitialized objects; that is the finalizer issue fixed separately in make finalize() a no-op for never-constructed module objects #1518.Overhead on the success path is within noise: ~1.15-1.24 us per call of
norm2(p)on both master and this branch.Checklist
R CMD checkstill passes all tests (the test files listed above, run locally)