Repository navigation
module object arguments are converted without any validity or class checks #1523
Description
When an R object is converted to a C++ class exposed by a module (as<T>, as<T&>, as<const T&>, as<T*>, as<const T*>, i.e. any function or method parameter of such a type, including // [[Rcpp::export]] functions), Rcpp does no checking at all. as_module_object_internal() just reads the object's .pointer binding and returns its address:
Rcpp/inst/include/Rcpp/api/meat/as.h
Lines 24 to 28 in 2cf7685
| inline void* as_module_object_internal(SEXP obj){ | |
| Environment env(obj) ; | |
| SEXP xp = env.get(".pointer") ; | |
| return R_ExternalPtrAddr(xp ); | |
| } |
inline void* as_module_object_internal(SEXP obj){
Environment env(obj) ;
SEXP xp = env.get(".pointer") ;
return R_ExternalPtrAddr(xp );
}That has several consequences:
- Objects of the wrong class are reinterpreted. Passing an instance of module class
Labelwhere aPoint&is expected hands theLabel*to C++ as aPoint*. Reads return garbage and writes corrupt the other object. - Deserialized objects segfault. After
saveRDS()/readRDS()(or sending an object to a parallel worker),.pointeris a NULL external pointer, which is then dereferenced. Calling a method on the same object gives a cleanexternal pointer is not validerror, because method dispatch goes throughXPtr::checked_get(). - Uninitialized objects (
new(Class)for a class without a default constructor) store a dummy environment in.pointer, and non-module arguments go throughas.environment()(so e.g.1looks up.pointerin the global environment). Recent R versions catch these inR_ExternalPtrAddr()'s type check; older ones return a garbage address.
Reproducible example
repro.cpp:
#include <Rcpp.h>
using namespace Rcpp;
class Point {
public:
Point(double x_, double y_) : x(x_), y(y_) {}
double x, y;
};
class Label {
public:
Label(std::string text_) : text(text_) {}
std::string text;
};
RCPP_EXPOSED_CLASS_NODECL(Point)
double norm2(const Point& p) {
return p.x * p.x + p.y * p.y;
}
void shift(Point& p, double dx) {
p.x += dx;
}
RCPP_MODULE(point_module) {
class_<Point>("Point")
.constructor<double, double>();
class_<Label>("Label")
.constructor<std::string>()
.field("text", &Label::text);
function("norm2", &norm2);
function("shift", &shift);
}repro.R:
Rcpp::sourceCpp("repro.cpp")
# 1. an object of another module class is reinterpreted as a Point
label <- new(Label, "hello")
cat("norm2(<Label>):", norm2(label), "\n")
shift(label, 1e6) # writes a double into the Label's std::string
cat("label$text after shift():", label$text, "\n")
# 2. a deserialized Point holds a NULL pointer, which is dereferenced
p <- new(Point, 3, 4)
cat("norm2(<Point>):", norm2(p), "\n")
p2 <- unserialize(serialize(p, NULL)) # e.g. saveRDS() then readRDS()
cat("norm2(<deserialized Point>):", norm2(p2), "\n")Output with R 4.6.1 (macOS 27, Apple clang 21, Rcpp 1.1.2.6 / current master):
norm2(<Label>): 0
label$text after shift():
norm2(<Point>): 25
*** caught segfault ***
address 0x0, cause 'invalid permissions'
Traceback:
1: .External(list(name = "InternalFunction_invoke", ...), <pointer: 0x78fad50930>, ...)
2: norm2(p2)
3: cat("norm2(<deserialized Point>):", norm2(p2), "\n")
An irrecoverable exception occurred. R is aborting now ...
Segmentation fault: 11 Rscript repro.R
The Label is silently accepted as a Point, shift() overwrites the string's internal representation (label$text is now empty), and the deserialized Point crashes the session.
I'll follow up with a PR that validates the object before handing out its address: it must be a module object, its .pointer must be a live external pointer (external pointer is not valid / C++ object not initialized otherwise, matching the method-call errors), and its class must be T or a class that derives from T through class_<>::derives<T>(). Anything else becomes an R error. The class check does change behavior for code that currently passes an object of an unrelated module class, including a C++ subclass exposed without .derives<>(), which only works today when the base class happens to sit at offset 0; that will need a reverse-dependency check.
Activity