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

Protection problems with C++ interfaces #712

Closed
krlmlr opened this Issue Jun 14, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@krlmlr
Contributor

krlmlr commented Jun 14, 2017

My package "bindrcpp" provides a C++ interface via // [[Rcpp::interfaces(cpp)]]. I was able to track down spurious segmentation faults (in dplyr, tidyverse/dplyr#2811) to (what I think is) a protection error in the auto-generated C++ wrappers for these interfaces.

The commit krlmlr/bindrcpp@6d1e2bd shows both the problem and the solution: In general, wrap() will perform allocation of R objects, and without extra shielding these can be garbage collected even before the wrapped function is called, because the latter has all SEXP in its signature.

After applying this particular commit, I'm not seeing test failures anymore, which before occurred after a few iterations. Happy to submit a PR.

CC @lionel-.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 14, 2017

Member

It's been a while but we did revise that aspect once or twice already.

Could you provide a reproducible example we could call before and after? With and without gctorture()?

Member

eddelbuettel commented Jun 14, 2017

It's been a while but we did revise that aspect once or twice already.

Could you provide a reproducible example we could call before and after? With and without gctorture()?

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 14, 2017

Contributor

Thanks. That commit is the reprex, gctorture2() is called inside the tests:

hub clone krlmlr/bindrcpp
cd bindrcpp
git checkout 6d1e2bd^ # omit ^ to see fixed behavior
R -q -e 'devtools::test(reporter = c("summary", "stop"))'
Contributor

krlmlr commented Jun 14, 2017

Thanks. That commit is the reprex, gctorture2() is called inside the tests:

hub clone krlmlr/bindrcpp
cd bindrcpp
git checkout 6d1e2bd^ # omit ^ to see fixed behavior
R -q -e 'devtools::test(reporter = c("summary", "stop"))'
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 14, 2017

Member

I am sorry but I don't use hub, or devtools.

And involving an entire different package strikes me as a little far away from "minimal".

Member

eddelbuettel commented Jun 14, 2017

I am sorry but I don't use hub, or devtools.

And involving an entire different package strikes me as a little far away from "minimal".

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 14, 2017

Contributor

Give me a minute to translate that for you. I seriously recommend checking out hub, though: https://github.com/github/hub

Contributor

krlmlr commented Jun 14, 2017

Give me a minute to translate that for you. I seriously recommend checking out hub, though: https://github.com/github/hub

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 14, 2017

Member

Yes, I read about it. So far exiting command-line and magit are good enough for me.

Member

eddelbuettel commented Jun 14, 2017

Yes, I read about it. So far exiting command-line and magit are good enough for me.

@lionel-

This comment has been minimized.

Show comment
Hide comment
@lionel-

lionel- Jun 14, 2017

Contributor

who needs hub when there's magit ;)

Contributor

lionel- commented Jun 14, 2017

who needs hub when there's magit ;)

@krlmlr

This comment has been minimized.

Show comment
Hide comment
@krlmlr

krlmlr Jun 14, 2017

Contributor

Thanks for the pointers, will check out magit.

Now, for the reprex:

  1. Install bindrcpp from https://github.com/krlmlr/bindrcpp/tree/a7759861e780fa84cf022f31809af3744599abea
  2. Run the script below, it should stop very soon
  3. Now install the fixed version from https://github.com/krlmlr/bindrcpp/tree/6d1e2bde24fd8a26a3911923cd2e6cd0a1a29180
  4. Run the script below, it should run until the end

If you install from a clone with R CMD INSTALL ., you need to remove src/*.o before switching to a different version.

library(bindrcpp)
gctorture2(29)

for (i in 1:1000) {

print(i)

env_cb <- bindrcpp:::cpp_create_environment(letters, "toupper")
env <- env_cb$env
env2_cb <- bindrcpp:::cpp_create_environment(LETTERS, "tolower", parent = env)
env2 <- env2_cb$env
stopifnot(identical(get("a", env2), "A"))
stopifnot(identical(get("x", env2), "X"))
stopifnot(identical(env2$a, NULL))
stopifnot(identical(env2$x, NULL))
stopifnot(identical(env2$B, "b"))
stopifnot(identical(env2$Y, "y"))
stopifnot(identical(length(ls(env2)), length(letters)))
env2$a <- "a"
stopifnot(identical(get("a", env2), "a"))

}
Contributor

krlmlr commented Jun 14, 2017

Thanks for the pointers, will check out magit.

Now, for the reprex:

  1. Install bindrcpp from https://github.com/krlmlr/bindrcpp/tree/a7759861e780fa84cf022f31809af3744599abea
  2. Run the script below, it should stop very soon
  3. Now install the fixed version from https://github.com/krlmlr/bindrcpp/tree/6d1e2bde24fd8a26a3911923cd2e6cd0a1a29180
  4. Run the script below, it should run until the end

If you install from a clone with R CMD INSTALL ., you need to remove src/*.o before switching to a different version.

library(bindrcpp)
gctorture2(29)

for (i in 1:1000) {

print(i)

env_cb <- bindrcpp:::cpp_create_environment(letters, "toupper")
env <- env_cb$env
env2_cb <- bindrcpp:::cpp_create_environment(LETTERS, "tolower", parent = env)
env2 <- env2_cb$env
stopifnot(identical(get("a", env2), "A"))
stopifnot(identical(get("x", env2), "X"))
stopifnot(identical(env2$a, NULL))
stopifnot(identical(env2$x, NULL))
stopifnot(identical(env2$B, "b"))
stopifnot(identical(env2$Y, "y"))
stopifnot(identical(length(ls(env2)), length(letters)))
env2$a <- "a"
stopifnot(identical(get("a", env2), "a"))

}
@jjallaire

This comment has been minimized.

Show comment
Hide comment
@jjallaire

jjallaire Jun 14, 2017

Member
Member

jjallaire commented Jun 14, 2017

eddelbuettel added a commit that referenced this issue Jun 14, 2017

Merge pull request #713 from RcppCore/bugfix/interfaces-param-shield
Add Shield around parameters in Rcpp::interfaces (fixes #712)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment