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

Nested unwind_protect() calls going through R can be very dangerous #326

Closed
DavisVaughan opened this issue Jul 26, 2023 · 0 comments · Fixed by #327
Closed

Nested unwind_protect() calls going through R can be very dangerous #326

DavisVaughan opened this issue Jul 26, 2023 · 0 comments · Fixed by #327

Comments

@DavisVaughan
Copy link
Member

Assume you have cpp11 0.4.4 installed, with a "working" version of the cpp11_should_unwind_protect R global option ("working" in the sense that if you enter a nested unwind-protect call, then it notices that there is an outer unwind-protect present and decides not to unwind-protect its function too).

In this case, you can still have big problems with the following chain of events:

  • cpp11 function B calls an R level callback (setting should_unwind_protect = FALSE)
  • R level callback itself calls cpp11 function A
  • Say that A sets up some complex C++ objects with destructors
  • Then that A ends up calling cpp11::stop() or something that longjmps, but should_unwind_protect = FALSE so it never set up a setjmp().
  • The longjmp jumps all the way back to B's setjmp(), completely bypassing any destructors needed by A

The big issue here is that A() and B() on their own can look very harmless, and like code that a package author would write without thinking twice about it.

I've come up with a reprex package to demonstrate this issue:
https://github.com/DavisVaughan/testcpp11unwind


A few options:

  • Possibly the BEGIN_CPP11 entry macro should always reset should_unwind_protect = TRUE (making sure it exists first). Then A's call to unwind_protect() would still use setjmp() and R_UnwindProtect().
  • Consider removing the cpp11_should_unwind_protect global option altogether

The cpp11_should_unwind_protect nest guard is used for two things:

  • For performance, i.e. you can wrap a tight loop where each iteration calls unwind_protect() in an outer unwind_protect() outside the loop so the protection is only set up once (mostly an issue with character vectors)
  • For safety, i.e. the following code doesn't work without the nest guard
[[cpp11::register]]
void test() {
  cpp11::unwind_protect([&] {
    cpp11::unwind_protect([&] {
      Rf_error("oh no!");
    });
  });
}

If test() is called from R:

  • It goes through .Call and our wrapper, which sets up the BEGIN_CPP11 and END_CPP11 macros
  • It calls unwind_protect() which calls R_UnwindProtect(), a C function!
  • Inside R_UnwindProtect(), we call unwind_protect()
  • From there we Rf_error()
  • The inner unwind_protect() catches that C error and promotes it to a C++ exception which is thrown.
  • That is thrown across C stack frames, because we are inside the outer R_UnwindProtect() and no try/catch was set up within that to catch the unwind exception. The only try/catch is that most outer one set up by the macros
  • That is UB and R crashes due to an uncaught exception
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant