[kevinushey]

Fixes #1521.

Module functions are exposed to R as function(...) .External(InternalFunction_invoke, fun_pointer, ...), and InternalFunction_invoke called the C++ function without checking how many arguments it got. Any parameter past the supplied ones was converted from an uninitialized slot of the cargs array, which usually segfaults (see #1521). Rcpp::InternalFunction goes through the same entry point and had the same problem.

InternalFunction_invoke only sees a CppFunctionBase, which has no nargs(), and adding a virtual there would change the vtable layout that Rcpp.so shares with already-compiled packages. So the arity travels in the external pointer's tag instead:

• inst/include/Rcpp/module/Module.h (Module::get_function): store fun->nargs() as an integer tag on the function pointer handed to R. This code is compiled into Rcpp.so (via Module__get_function), so module functions in packages built against older Rcpp headers get the check too.
• inst/include/Rcpp/InternalFunction.h: both constructors store sizeof...(T) / sizeof...(Args) the same way.
• src/module.cpp (InternalFunction_invoke): when the tag is an integer scalar and fewer arguments were supplied, raise incorrect number of arguments: expecting N, got M. Pointers without a tag (e.g. an InternalFunction created 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 formals list longer than the C++ parameter list), so I left it alone. The check covers the formals path too, since formals<- only rewrites the R wrapper's arguments: a formals list 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() (the formals<- method for C++Function, used for function(name, fun, formals)) rewrote body(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 is if (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 the formals path was #nocov and untested until the new foo_formals tests 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 with covr::package_coverage(): a module function with formals fails 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()), with formals defaults, and with a formals list shorter than the parameter list.
• inst/tinytest/test_internal_function.R and test_internal_function_cpp11.R: the function-pointer and std::function InternalFunction constructors.

Verification:

• The reprex from calling a module function with too few arguments reads uninitialized memory #1521 segfaults (exit code 139) against current master and prints result: incorrect number of arguments: expecting 2, got 1 against this branch.
• The same module, compiled against master's headers and loaded into a session using this branch's Rcpp.so, also gets the error (the tag is set by Rcpp.so).
• Against master the new tests segfault (test_module.R, test_internal_function_cpp11.R) or fail (test_internal_function.R). On this branch test_module.R, test_modref.R, test_internal_function.R, test_internal_function_cpp11.R, test_function.R, test_attributes.R, test_expose_class.R and test_module_client_package.R all pass with RunAllRcppTests=yes RunVerboseRcppTests=yes (R 4.6.1, macOS).

Checklist

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

[eddelbuettel]

Is it a potential problem that we assume no other use of the 'tag' field would arise? Presumably in the context of Modules if may be fine. (I have used tags to store type information ... but that was regular XPtr use.)

[kevinushey]

Good question. I think it's safe here, because the check only looks at the tag of the pointer passed as the first argument of .External(InternalFunction_invoke, ...). Those pointers come from just two places, both in Rcpp: Module::get_function() and the InternalFunction constructors. Neither has ever set a tag before (it was always R_NilValue, back through the generated pre-C++11 constructors), and R code can't set an external pointer's tag. XPtrs you tag yourself never reach this path, so they aren't affected. A GitHub code search for InternalFunction_invoke also turns up only copies and forks of Rcpp, with no outside callers.

If one of these pointers did end up with some other tag, the check skips anything that isn't a length-one integer and behaves as before. The only bad case is a foreign scalar-integer tag larger than the number of supplied arguments, and that gives a spurious "incorrect number of arguments" error, not a crash. If you'd like to rule that out too, I can make the tag self-identifying, e.g. a named integer c(nargs = N), with the check also testing the name.

[eddelbuettel]

Yep, I think we should be good here. While I have use tags to store type info that was never via Modules and always via direct XPtr use.

[eddelbuettel]

LGTM (and to the LLM I had look at it 😉 )

[eddelbuettel]

FYI I also approved #1518 and #1520 (though I could of course let the LLM look at it too).