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 function argument to rcpp_set_stack_trace() #706

Merged
merged 3 commits into from Jun 5, 2017

Conversation

Projects
None yet
5 participants
@krlmlr
Contributor

krlmlr commented Jun 4, 2017

which may be gc-ed otherwise.

I was unable to create a reproducible example for this problem, but I have two stack traces from test runs for dplyr with valgrind and gctorture enabled that indicate that this may be the source of a problem. I'm not seeing these errors anymore after applying this fix.

Show outdated Hide outdated inst/NEWS.Rd
@@ -20,6 +20,10 @@
\item Automatically register init functions for Rcpp Modules (\ghpr{705}
addressing \ghit{704}).
}
\item Bug fixes:

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 4, 2017

Member

There has never been an entry Bug fixes. Please place under Rcpp API.

@eddelbuettel

eddelbuettel Jun 4, 2017

Member

There has never been an entry Bug fixes. Please place under Rcpp API.

Show outdated Hide outdated src/barrier.cpp
@@ -121,7 +121,9 @@ SEXP rcpp_get_stack_trace() {
// [[Rcpp::register]]
SEXP rcpp_set_stack_trace(SEXP e) {
PROTECT(e);

This comment has been minimized.

@eddelbuettel

eddelbuettel Jun 4, 2017

Member

Should this be Rcpp::Shield<> instead?

@eddelbuettel

eddelbuettel Jun 4, 2017

Member

Should this be Rcpp::Shield<> instead?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 4, 2017

Member

Also, remember that we (generally) prefer Issue tickets prior to PRs...

Member

eddelbuettel commented Jun 4, 2017

Also, remember that we (generally) prefer Issue tickets prior to PRs...

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 4, 2017

Contributor

Next time I have a reprex I file an issue first. Promise ;-)

Contributor

krlmlr commented Jun 4, 2017

Next time I have a reprex I file an issue first. Promise ;-)

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 4, 2017

Codecov Report

Merging #706 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #706   +/-   ##
=======================================
  Coverage   89.57%   89.57%           
=======================================
  Files          66       66           
  Lines        3588     3588           
=======================================
  Hits         3214     3214           
  Misses        374      374
Impacted Files Coverage Δ
inst/include/Rcpp/exceptions.h 50% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7687508...66cf2c6. Read the comment docs.

codecov-io commented Jun 4, 2017

Codecov Report

Merging #706 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #706   +/-   ##
=======================================
  Coverage   89.57%   89.57%           
=======================================
  Files          66       66           
  Lines        3588     3588           
=======================================
  Hits         3214     3214           
  Misses        374      374
Impacted Files Coverage Δ
inst/include/Rcpp/exceptions.h 50% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7687508...66cf2c6. Read the comment docs.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 5, 2017

Member

Thanks for quickly making the changes. Does it still pass your local test?

Member

eddelbuettel commented Jun 5, 2017

Thanks for quickly making the changes. Does it still pass your local test?

@krlmlr krlmlr referenced this pull request Jun 5, 2017

Closed

Reproducible segfault #2811

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Jun 5, 2017

Contributor

Thanks for catching this! I don't think this is quite the right change, though.

Normally, we should be able to operate under the assumption that SEXP arguments received by functions are already protected by someone in a parent scope; e.g. this is what is done for methods called from R through the .Call() interface and I think we should preserve these semantics in other C++-specific code as well.

It looks to me like the real issue here is that the SEXP returned by calls to stack_trace() is not protected, e.g. here:

rcpp_set_stack_trace(stack_trace());

rcpp_set_stack_trace(stack_trace(file,line));

And so the returned SEXP from stack_trace() should be protected before being sent to those function calls.

Contributor

kevinushey commented Jun 5, 2017

Thanks for catching this! I don't think this is quite the right change, though.

Normally, we should be able to operate under the assumption that SEXP arguments received by functions are already protected by someone in a parent scope; e.g. this is what is done for methods called from R through the .Call() interface and I think we should preserve these semantics in other C++-specific code as well.

It looks to me like the real issue here is that the SEXP returned by calls to stack_trace() is not protected, e.g. here:

rcpp_set_stack_trace(stack_trace());

rcpp_set_stack_trace(stack_trace(file,line));

And so the returned SEXP from stack_trace() should be protected before being sent to those function calls.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 5, 2017

Member

Next time I have a reprex I file an issue first. Promise ;-)

Well whether you want it or not we're having the review now :)

Member

eddelbuettel commented Jun 5, 2017

Next time I have a reprex I file an issue first. Promise ;-)

Well whether you want it or not we're having the review now :)

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 5, 2017

Contributor

@kevinushey: Good point, thanks for chiming in.

@eddelbuettel: I'm fine with that.

Contributor

krlmlr commented Jun 5, 2017

@kevinushey: Good point, thanks for chiming in.

@eddelbuettel: I'm fine with that.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 5, 2017

Contributor

Concerning the local test: I can no longer replicate the original heisenbug. I'll run a test on dplyr using this branch of Rcpp with valgrind and gctorture.

Contributor

krlmlr commented Jun 5, 2017

Concerning the local test: I can no longer replicate the original heisenbug. I'll run a test on dplyr using this branch of Rcpp with valgrind and gctorture.

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 5, 2017

Contributor

FWIW, Rf_ScalarString() does protect its argument.

Contributor

krlmlr commented Jun 5, 2017

FWIW, Rf_ScalarString() does protect its argument.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 5, 2017

Member

FWIW, Rf_ScalarString() does protect its argument.

Meaning we'd no longer need this? Colour me confused.

In any event, this is unlikely to do harm, and as @kevinushey pointed out both a good catch anyway and now in a better spot so I'll merge.

Member

eddelbuettel commented Jun 5, 2017

FWIW, Rf_ScalarString() does protect its argument.

Meaning we'd no longer need this? Colour me confused.

In any event, this is unlikely to do harm, and as @kevinushey pointed out both a good catch anyway and now in a better spot so I'll merge.

@eddelbuettel eddelbuettel merged commit 0725728 into RcppCore:master Jun 5, 2017

1 check passed

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

@krlmlr krlmlr deleted the krlmlr:b-stack-trace-protect branch Jun 5, 2017

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 5, 2017

Contributor

I was pointing out that some functions seem to be caller-protected, others callee-protected. But then Rf_ScalarString() may just be the big exception of the rule, I don't know.

Contributor

krlmlr commented Jun 5, 2017

I was pointing out that some functions seem to be caller-protected, others callee-protected. But then Rf_ScalarString() may just be the big exception of the rule, I don't know.

@lionel-

This comment has been minimized.

Show comment
Hide comment
@lionel-

lionel- Jun 5, 2017

Contributor

The rule in the R source code is that the caller protects arguments and the callee can expect its arguments to be protected. There are documented exceptions, e.g. setAttrib().

Contributor

lionel- commented Jun 5, 2017

The rule in the R source code is that the caller protects arguments and the callee can expect its arguments to be protected. There are documented exceptions, e.g. setAttrib().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment