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.

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

protect call inside Rcpp::Environment<>::namespace_env() #1010

Merged
merged 4 commits into from Nov 2, 2019

Conversation

@romainfrancois
Copy link
Contributor

romainfrancois commented Oct 31, 2019

The result of Rf_lang2() is not protected so might be deleted by a garbage collection further down the line.

romainfrancois added a commit to tidyverse/dplyr that referenced this pull request Oct 31, 2019
@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Oct 31, 2019

LGTM!

It looks like there are a number of places where we fail to protect the created call, so we might want to audit all of those usages and try to fix them up.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Oct 31, 2019

Quick ag search for Rf_lang2 in the headers find most uses protected by now. Besides the one identified here, I see one more in generated/Function__opearator.h and one in r_cast.h.

Environment.h has another related use with Rf_lang3 and one with Rf_lang4; both probably also need protection.

@kevinushey

This comment has been minimized.

Copy link
Contributor

kevinushey commented Oct 31, 2019

There are also a couple cases where we use Rf_mkString() within calls to Rf_lang*(); IIRC this can be dangerous because (depending on the order the compiler decides to materialize function arguments) this can lead to (very rare) GC errors as well.

We should likely pull those out and protect those as well.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Oct 31, 2019

We could split those two tasks between us :)

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Nov 2, 2019

No 'change to worse' in reverse depends: RcppCore/rcpp-logs@d855acb so merging.

@eddelbuettel eddelbuettel merged commit 47daf66 into RcppCore:master Nov 2, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eddelbuettel added a commit that referenced this pull request Nov 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.