Skip to content

validate module objects before converting them to c++ classes - #1524

Open
kevinushey wants to merge 2 commits into
masterfrom
bugfix/module-object-as-checks
Open

kevinushey wants to merge 2 commits into
masterfrom
bugfix/module-object-as-checks

Conversation

@kevinushey

Copy link
Copy Markdown
Contributor

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 .pointer address 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 raises Expecting a C++ module object for non-module arguments, C++ object not initialized. (Missing default constructor?) for the dummy object of a class without a default constructor, and external pointer is not valid for a NULL pointer, e.g. after saveRDS()/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).
    • A new overload 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 through class_<>::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-object as<> dispatch passes typeid(T).name() to the new overload.

Everything is header code compiled into the calling package; nothing in Rcpp.so or 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 a Base& 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 a ModuleBase/ModuleDerived pair added to cpp/Module.cpp):

  • wrong class, for both a module function and a // [[Rcpp::export]] function
  • deserialized object, uninitialized object, and a non-module argument
  • an object of a class that derives() from the expected class is accepted

Verification:

  • 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$text is left intact):

    norm2(<Label>):              Expecting an object of C++ class 'Point', got an object of class 'Label'
    shift(<Label>, 1e6):         Expecting an object of C++ class 'Point', got an object of class 'Label'
    label$text:                  hello
    norm2(<deserialized Point>): external pointer is not valid
    norm2(1):                    Expecting an object of C++ class 'Point'
    norm2(<Point>):              25
    
  • 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.R and test_as.R pass with RunAllRcppTests=yes RunVerboseRcppTests=yes (R 4.6.1, macOS). test_module.R still prints Error: 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

  • Code compiles correctly
  • R CMD check still passes all tests (the test files listed above, run locally)
  • Preferably, new tests were added which fail without the change (without the fix, the new tests segfault)
  • Document the changes by file in ChangeLog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

module object arguments are converted without any validity or class checks

1 participant