Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rewrite no_init() and other functions to reduce false positives #893

Merged
merged 6 commits into from
Aug 27, 2018
Merged

Rewrite no_init() and other functions to reduce false positives #893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5dbfca1
Explicitly protect temporary vector in no_init()
krlmlr Aug 18, 2018
4c4721a
Fix more false rchk positives
krlmlr Aug 18, 2018
3fa6ca0
make Rcpp_unprotect a void function
eddelbuettel Aug 18, 2018
72f4db1
Add comments
krlmlr Aug 19, 2018
3d5e62f
Tweaks
krlmlr Aug 26, 2018
b735942
Support matrix case
krlmlr Aug 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion inst/include/Rcpp/api/meat/Rcpp_eval.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ inline SEXP Rcpp_fast_eval(SEXP expr, SEXP env) {
inline SEXP Rcpp_eval(SEXP expr, SEXP env) {

// 'identity' function used to capture errors, interrupts
SEXP identity = Rf_findFun(::Rf_install("identity"), R_BaseNamespace);
Shield<SEXP> identity(Rf_findFun(::Rf_install("identity"), R_BaseNamespace));

if (identity == R_UnboundValue) {
stop("Failed to find 'base::identity()'");
Expand Down
2 changes: 1 addition & 1 deletion inst/include/Rcpp/exceptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ namespace internal {
inline bool is_Rcpp_eval_call(SEXP expr) {
SEXP sys_calls_symbol = Rf_install("sys.calls");
SEXP identity_symbol = Rf_install("identity");
SEXP identity_fun = Rf_findFun(identity_symbol, R_BaseEnv);
Shield<SEXP> identity_fun(Rf_findFun(identity_symbol, R_BaseEnv));
SEXP tryCatch_symbol = Rf_install("tryCatch");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does rchk warn about the unprotected use of 'install' here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: krlmlr/RcppNoInitTest@38aa117#diff-7f4f9d499ac2cdf819002146b21b8e18L24

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should the PR cover that too then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was confused. No, rchk knows that the Rf_install() result is protected already, but Rf_install() could be an allocation (in theory) which could clean up the result of Rf_findFun().

SEXP evalq_symbol = Rf_install("evalq");

Expand Down
4 changes: 2 additions & 2 deletions inst/include/Rcpp/protection/Shelter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ namespace Rcpp {

inline SEXP operator()(SEXP x){
nprotected++;
return PROTECT(x) ;
return Rcpp_protect(x) ;
}

~Shelter(){
UNPROTECT(nprotected) ;
Rcpp_unprotect(nprotected) ;
nprotected = 0 ;
}

Expand Down
8 changes: 7 additions & 1 deletion inst/include/Rcpp/protection/Shield.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,18 @@ namespace Rcpp{
return x ;
}

inline void Rcpp_unprotect(int i){
// Prefer this function over UNPROTECT() in Rcpp so that all
// balance checks errors by rchk are contained at one location (#892)
UNPROTECT(i);
}

template <typename T>
class Shield{
public:
Shield( SEXP t_) : t(Rcpp_protect(t_)){}
~Shield(){
if( t != R_NilValue ) UNPROTECT(1) ;
if( t != R_NilValue ) Rcpp_unprotect(1) ;
}

operator SEXP() const { return t; }
Expand Down
12 changes: 10 additions & 2 deletions inst/include/Rcpp/vector/no_init.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ class no_init_vector {

template <int RTYPE, template <class> class StoragePolicy >
operator Vector<RTYPE, StoragePolicy>() const {
return Rf_allocVector(RTYPE, size) ;
// Explicitly protect temporary vector to avoid false positive
// with rchk (#892)
Shield<SEXP> x(Rf_allocVector(RTYPE, size));
Vector<RTYPE, PreserveStorage> ret(x);
return ret;
}

private:
Expand All @@ -58,7 +62,11 @@ class no_init_matrix {

template <int RTYPE, template <class> class StoragePolicy >
operator Matrix<RTYPE, StoragePolicy>() const {
return Rf_allocMatrix(RTYPE, nr, nc);
// Explicitly protect temporary matrix to avoid false positive
// with rchk (#892)
Shield<SEXP> x(Rf_allocMatrix(RTYPE, nr, nc));
Matrix<RTYPE, PreserveStorage> ret(x);
return ret;
}

private:
Expand Down