Remove unsafe usage of Rf_eval (close #498) #523

Merged
merged 1 commit into from Aug 1, 2016

Projects

None yet

4 participants

@thirdwing
Member

@kevinushey

@coatless coatless and 2 others commented on an outdated diff Jul 31, 2016
inst/include/Rcpp/api/meat/Rcpp_eval.h
@@ -54,7 +54,7 @@ inline SEXP Rcpp_eval(SEXP expr, SEXP env) {
SET_TAG(CDDR(CDR(call)), ::Rf_install("interrupt"));
// execute the call
- Shield<SEXP> res(::Rf_eval(call, R_GlobalEnv));
+ Shield<SEXP> res(::Rf_eval(call, env));
@coatless
coatless Jul 31, 2016 Contributor

Why the switch from R_GlobalEnv to env here but not on line 69?

Shield<SEXP> conditionMessage(::Rf_eval(conditionMessageCall, R_GlobalEnv));
@thirdwing
thirdwing Jul 31, 2016 Member

Actually this switch is not necessary, since we already have the env from evalq. I involve this when trying to simplify Rcpp_eval. I will update.

@kevinushey
kevinushey Aug 1, 2016 Contributor

The evalq call itself might be unneeded, really -- we could probably omit constructing of the evalq bit and just evaluate the tryCatch expression in env.

We want to ensure that expr is evaluated in env, but the other usages likely want to just be evaluated in the global environment.

@eddelbuettel
Member

Thank you -- nice work. Is that "all of them" or just the ones you thought we could safely change? I seem to recall @kevinushey mentioning that maybe some needed to remain or something to that extend. Or am I misremembering? Anybody to second the thumbs-up?

@kevinushey
Contributor
kevinushey commented Aug 1, 2016 edited

Looks great, thanks! @eddelbuettel I was thinking that the Rf_evals e.g. here:

https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/exceptions.h#L197
https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/exceptions.h#L326

probably don't want to be touched, as that's code that will get called in our try-catch macros, e.g. at

https://github.com/RcppCore/Rcpp/blob/master/inst/include/Rcpp/macros/macros.h#L37-L57

and so Rcpp_evals within there would be executed outside the 'safety' of a try-catch block, IIUC.

This PR doesn't touch those usages so I think that's okay.

@eddelbuettel
Member

Thank you both -- in it goes! And I resolve to do a full rev.dep check tomorrow. Worst case we revert.

@eddelbuettel eddelbuettel merged commit 2013d3a into RcppCore:master Aug 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel
Member

Just for the record it passed the rev.dep with flying colours as expected.

@eddelbuettel eddelbuettel added a commit that referenced this pull request Aug 2, 2016
@eddelbuettel eddelbuettel correction to NEWS entry
cf discussion in #523 commit log on GH
1dc7a84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment