GeezerVison is showing github.com — check module and internal function arity before invoking (#1522) · RcppCore/Rcpp@3f687ae
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: ChangeLog +20Lines changed: 20 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | + | | 2 | + | | 3 | + * src/module.cpp (InternalFunction_invoke): Raise an error when fewer | | 4 | + arguments are supplied than the arity stored in the function pointer's | | 5 | + tag, instead of reading the missing ones from uninitialized memory | | 6 | + (#1521) | | 7 | + * inst/include/Rcpp/module/Module.h (Module::get_function): Store the | | 8 | + arity of module functions in the pointer tag | | 9 | + * inst/include/Rcpp/InternalFunction.h: Idem for InternalFunction | | 10 | + * inst/tinytest/cpp/Module.cpp: Add module functions with formals | | 11 | + * inst/tinytest/test_module.R: Test calls with too few arguments | | 12 | + * inst/tinytest/test_internal_function.R: Idem | | 13 | + * inst/tinytest/test_internal_function_cpp11.R: Idem | | 14 | + | | 15 | + * R/00_classes.R (.cppfunction_formals_gets): Find the .External() call | | 16 | + in the body of the function instead of assuming it is the second | | 17 | + element, which it is not once coverage tools have instrumented the body | | 18 | + * R/00_classes.R (.cppfunction_set_external_args): New helper | | 19 | + * inst/tinytest/test_module.R: Test it with an instrumented body | | 20 | + | 1 | 21 | | 2 | 22 |
| 3 | 23 | * DESCRIPTION (Version, Date): Roll micro version and date | |
Collapse file Copy file name to clipboardExpand all lines: R/00_classes.R +16-6Lines changed: 16 additions & 6 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -117,13 +117,23 @@ setClass( "C++Function", | 117 | 117 | contains = "function" | 118 | 118 | ) | 119 | 119 |
| 120 | | -.cppfunction_formals_gets <- function (fun, envir = environment(fun), value) { | 121 | | - bd <- body(fun) | 122 | | - b2 <- bd[[2L]] | 123 | | - for( i in seq_along(value) ){ | 124 | | - b2[[3L+i]] <- as.name( names(value)[i] ) | | 120 | +## passes the arguments named in 'names' on from the .External(entry, | | 121 | +## pointer, ...) call in expr; the call is searched for rather than assumed | | 122 | +## to be a top-level statement, as coverage tools wrap each statement | | 123 | +.cppfunction_set_external_args <- function( expr, names ){ | | 124 | + if( !is.call( expr ) ) | | 125 | + return( expr ) | | 126 | + if( identical( expr[[1L]], as.name( ".External" ) ) ) | | 127 | + return( as.call( c( as.list( expr )[1:3], lapply( names, as.name ) ) ) ) | | 128 | + for( i in seq_along( expr )[-1L] ){ | | 129 | + if( is.call( expr[[i]] ) ) | | 130 | + expr[[i]] <- .cppfunction_set_external_args( expr[[i]], names ) | 125 | 131 | } | 126 | | - bd[[2]] <- b2 | | 132 | + expr | | 133 | +} | | 134 | + | | 135 | +.cppfunction_formals_gets <- function (fun, envir = environment(fun), value) { | | 136 | + bd <- .cppfunction_set_external_args( body(fun), names(value) ) | 127 | 137 | f <- fun@.Data | 128 | 138 | formals(f) <- value | 129 | 139 | body(f) <- bd | |
Collapse file Copy file name to clipboardExpand all lines: inst/include/Rcpp/InternalFunction.h +8-2Lines changed: 8 additions & 2 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -35,18 +35,24 @@ namespace Rcpp{ | 35 | 35 |
| 36 | 36 | RCPP_GENERATE_CTOR_ASSIGN(InternalFunction_Impl) | 37 | 37 |
| | 38 | + // the arity goes in the pointer tag, where InternalFunction_invoke | | 39 | + // checks it against the number of supplied arguments | | 40 | + | 38 | 41 | template <typename RESULT_TYPE, typename... Args> | 39 | 42 | InternalFunction_Impl(const std::function<RESULT_TYPE(Args...)> &fun) { | | 43 | + Shield<SEXP> arity(Rf_ScalarInteger(static_cast<int>(sizeof...(Args)))); | 40 | 44 | set( | 41 | 45 | XPtr<Rcpp::InternalFunctionWithStdFunction::CppFunctionBaseFromStdFunction<RESULT_TYPE, Args...> >( | 42 | 46 | new Rcpp::InternalFunctionWithStdFunction::CppFunctionBaseFromStdFunction<RESULT_TYPE, Args...>(fun), | 43 | | - false | | 47 | + false, | | 48 | + arity | 44 | 49 | ) | 45 | 50 | ); | 46 | 51 | } | 47 | 52 | template <typename RESULT_TYPE, typename... T> | 48 | 53 | InternalFunction_Impl(RESULT_TYPE (*fun)(T...)) { | 49 | | - set(XPtr<CppFunctionN<RESULT_TYPE, T...> >(new CppFunctionN<RESULT_TYPE, T...>(fun), true)); | | 54 | + Shield<SEXP> arity(Rf_ScalarInteger(static_cast<int>(sizeof...(T)))); | | 55 | + set(XPtr<CppFunctionN<RESULT_TYPE, T...> >(new CppFunctionN<RESULT_TYPE, T...>(fun), true, arity)); | 50 | 56 | } | 51 | 57 |
| 52 | 58 | void update(SEXP){} | |
Collapse file Copy file name to clipboardExpand all lines: inst/include/Rcpp/module/Module.h +5-1Lines changed: 5 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -164,8 +164,12 @@ namespace Rcpp { | 164 | 164 | } | 165 | 165 | std::string sign ; | 166 | 166 | fun->signature( sign, name_.data() ) ; | | 167 | + | | 168 | + // checked against the number of supplied arguments by InternalFunction_invoke | | 169 | + Shield<SEXP> arity( Rf_ScalarInteger( fun->nargs() ) ) ; | | 170 | + | 167 | 171 | return List::create( | 168 | | - XPtr<CppFunction>( fun, false ), | | 172 | + XPtr<CppFunction>( fun, false, arity ), | 169 | 173 | fun->is_void(), | 170 | 174 | fun->docstring, | 171 | 175 | sign, | |
Collapse file Copy file name to clipboardExpand all lines: inst/tinytest/cpp/Module.cpp +4Lines changed: 4 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -166,6 +166,10 @@ RCPP_MODULE(demoModule) { | 166 | 166 | function("bla1" , &bla1 ); | 167 | 167 | function("bla2" , &bla2 ); | 168 | 168 |
| | 169 | + // formals with defaults, and formals shorter than the parameter list | | 170 | + function("foo_formals", &foo, List::create(_["x"] = 2, _["y"] = 10.0)); | | 171 | + function("foo_short_formals", &foo, List::create(_["x"] = 2)); | | 172 | + | 169 | 173 | function("test_reference", test_reference); | 170 | 174 | function("test_const_reference", test_const_reference); | 171 | 175 | function("test_const", test_const); | |
Collapse file Copy file name to clipboardExpand all lines: inst/tinytest/test_internal_function.R +4Lines changed: 4 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -24,6 +24,10 @@ Rcpp::sourceCpp("cpp/InternalFunction.cpp") | 24 | 24 | fun <- getAdd() | 25 | 25 | expect_equal( fun(10,32), 42 ) | 26 | 26 |
| | 27 | +## too few arguments raise an error rather than reading the missing ones | | 28 | +## from uninitialized memory (#1521) | | 29 | +expect_error( fun(10), "expecting 2, got 1" ) | | 30 | + | 27 | 31 | # test.internal_function_concatenate <- function(){ | 28 | 32 | fun <- getConcatenate() | 29 | 33 | expect_equal( fun("Hello"," World"), "Hello World" ) |
Collapse file Copy file name to clipboardExpand all lines: inst/tinytest/test_internal_function_cpp11.R +4Lines changed: 4 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -29,3 +29,7 @@ expect_equal( fun(38), 42 ) | 29 | 29 | # test.internal_function_concatenate <- function(){ | 30 | 30 | fun <- getConcatenate() | 31 | 31 | expect_equal( fun("Hello"," World"), "Hello World" ) | | 32 | + | | 33 | +## too few arguments raise an error rather than reading the missing ones | | 34 | +## from uninitialized memory (#1521) | | 35 | +expect_error( fun("Hello"), "expecting 2, got 1" ) |
Collapse file Copy file name to clipboardExpand all lines: inst/tinytest/test_module.R +15Lines changed: 15 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| 41 | 41 | expect_true(is_void(bla1@.Data)) | 42 | 42 | expect_true(is_void(bla2@.Data)) | 43 | 43 |
| | 44 | +## module functions must be called with at least as many arguments as | | 45 | +## the C++ function takes, rather than reading the missing ones from | | 46 | +## uninitialized memory (#1521) | | 47 | +expect_error( foo( 2L ), "expecting 2, got 1" ) | | 48 | +expect_error( bar(), "expecting 1, got 0" ) | | 49 | +expect_equal( foo_formals(), 20.0 ) | | 50 | +expect_equal( foo_formals( y = 3.0 ), 6.0 ) | | 51 | +expect_error( foo_short_formals(), "expecting 2, got 1" ) | | 52 | + | | 53 | +## formals<- finds the .External() call wherever it is in the body, e.g. | | 54 | +## inside the block that coverage tools (covr) wrap each statement in | | 55 | +wrapped <- quote({ if (TRUE) { covr:::count("key"); .External(entry, pointer, ...) } }) | | 56 | +expect_equal( deparse( Rcpp:::.cppfunction_set_external_args( wrapped, c("x", "y") ) ), | | 57 | + deparse( quote({ if (TRUE) { covr:::count("key"); .External(entry, pointer, x, y) } }) ) ) | | 58 | + | 44 | 59 | w <- new( ModuleWorld ) | 45 | 60 | expect_equal( w$greet(), "hello" ) | 46 | 61 | w$set( "hello world" ) | |
Collapse file Copy file name to clipboardExpand all lines: src/module.cpp +12Lines changed: 12 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -119,6 +119,18 @@ BEGIN_RCPP | 119 | 119 | SEXP p = CDR(args); | 120 | 120 | XP_Function fun(CAR(p)); p = CDR(p); | 121 | 121 | UNPACK_EXTERNAL_ARGS(cargs,p) | | 122 | + | | 123 | + // Module::get_function() and InternalFunction store the arity in the | | 124 | + // tag; without this check, parameters beyond those supplied would be | | 125 | + // read from uninitialized slots of cargs | | 126 | + SEXP arity = R_ExternalPtrTag(fun); | | 127 | + if (TYPEOF(arity) == INTSXP && Rf_xlength(arity) == 1 && nargs < INTEGER(arity)[0]) | | 128 | + Rcpp::stop( | | 129 | + "incorrect number of arguments: expecting %d, got %d", | | 130 | + INTEGER(arity)[0], | | 131 | + nargs | | 132 | + ); | | 133 | + | 122 | 134 | return fun->operator()(cargs); | 123 | 135 | END_RCPP | 124 | 136 | } | |
|