Repository navigation
check module and internal function arity before invoking - #1522
Open
kevinushey wants to merge 2 commits into
Open
kevinushey wants to merge 2 commits into
kevinushey wants to merge 2 commits into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1521.
Module functions are exposed to R as
function(...) .External(InternalFunction_invoke, fun_pointer, ...), andInternalFunction_invokecalled the C++ function without checking how many arguments it got. Any parameter past the supplied ones was converted from an uninitialized slot of thecargsarray, which usually segfaults (see #1521).Rcpp::InternalFunctiongoes through the same entry point and had the same problem.InternalFunction_invokeonly sees aCppFunctionBase, which has nonargs(), and adding a virtual there would change the vtable layout thatRcpp.soshares with already-compiled packages. So the arity travels in the external pointer's tag instead:inst/include/Rcpp/module/Module.h(Module::get_function): storefun->nargs()as an integer tag on the function pointer handed to R. This code is compiled intoRcpp.so(viaModule__get_function), so module functions in packages built against older Rcpp headers get the check too.inst/include/Rcpp/InternalFunction.h: both constructors storesizeof...(T)/sizeof...(Args)the same way.src/module.cpp(InternalFunction_invoke): when the tag is an integer scalar and fewer arguments were supplied, raiseincorrect number of arguments: expecting N, got M. Pointers without a tag (e.g. anInternalFunctioncreated by a package built against older headers) behave as before.Extra arguments are still ignored, as before; making that an error could break existing callers (e.g. a
formalslist longer than the C++ parameter list), so I left it alone. The check covers theformalspath too, sinceformals<-only rewrites the R wrapper's arguments: aformalslist shorter than the C++ parameter list now errors on call instead of reading uninitialized memory.Also, in a follow-up commit,
R/00_classes.R:.cppfunction_formals_gets()(theformals<-method forC++Function, used forfunction(name, fun, formals)) rewrotebody(fun)[[2L]], assuming it is the.External()call. Under covr's instrumentation,package_coverage()wraps each statement of the wrapper template in.get_Module_function(), so element 2 isif (TRUE) { covr:::count(...); .External(...) }. The rewrite then missed the call, and calling the function failed with'...' used in an incorrect context. That is already the case on master, but theformalspath was#nocovand untested until the newfoo_formalstests here made the covr job fail. A new helper,.cppfunction_set_external_args(), now finds the.External()call wherever it is and rebuilds its arguments. There is a direct test with a covr-style wrapped body, and I checked the fix withcovr::package_coverage(): a module function withformalsfails as above on master and works on this branch.Tests:
inst/tinytest/test_module.R: module functions called with too few arguments (foo(2L),bar()), withformalsdefaults, and with aformalslist shorter than the parameter list.inst/tinytest/test_internal_function.Randtest_internal_function_cpp11.R: the function-pointer andstd::functionInternalFunctionconstructors.Verification:
result: incorrect number of arguments: expecting 2, got 1against this branch.Rcpp.so, also gets the error (the tag is set byRcpp.so).test_module.R,test_internal_function_cpp11.R) or fail (test_internal_function.R). On this branchtest_module.R,test_modref.R,test_internal_function.R,test_internal_function_cpp11.R,test_function.R,test_attributes.R,test_expose_class.Randtest_module_client_package.Rall pass withRunAllRcppTests=yes RunVerboseRcppTests=yes(R 4.6.1, macOS).Checklist
R CMD checkstill passes all tests (the module, function and internal function test files listed above, run locally)