Skip to content

Q: Is it safe to mess with option 'cpp11_preserve_xptr'? #268

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

Closed
HenrikBengtsson opened this issue Mar 10, 2022 · 5 comments · Fixed by #331
Closed

Q: Is it safe to mess with option 'cpp11_preserve_xptr'? #268

HenrikBengtsson opened this issue Mar 10, 2022 · 5 comments · Fixed by #331

Comments

@HenrikBengtsson
Copy link

While trying to troubleshoot some segfaults, I stumbled upon the R option cpp11_preserve_xptr that cpp11 uses internally;

private:
// The preserved list singleton is stored in a XPtr within an R global option.
//
// It is not constructed as a static variable directly since many
// translation units may be compiled, resulting in unrelated instances of each
// static variable.
//
// We cannot store it in the cpp11 namespace, as cpp11 likely will not be loaded by
// packages.
// We cannot store it in R's global environment, as that is against CRAN
// policies.
// We instead store it as an XPtr in the global options, which avoids issues
// both copying and serializing.
static SEXP get_preserve_xptr_addr() {
static SEXP preserve_xptr_sym = Rf_install("cpp11_preserve_xptr");
SEXP preserve_xptr = Rf_GetOption1(preserve_xptr_sym);
if (TYPEOF(preserve_xptr) != EXTPTRSXP) {
return R_NilValue;
}
auto addr = R_ExternalPtrAddr(preserve_xptr);
if (addr == nullptr) {
return R_NilValue;
}
return static_cast<SEXP>(addr);
}

To be clear, I have no idea what it does, but looking at it's value, e.g.

> getOption("cpp11_preserve_xptr")
<pointer: 0x55555a020518>

is see it's an external pointer.

My question is, is the cpp11 code robust to anything that messes with this option? For example, can it safely be unset:

options(cpp11_preserve_xptr = NULL)

at any time? Can you imagine a scenario where changing it's value would wreak havoc?

@romainfrancois
Copy link
Collaborator

I would say the danger is limited as it's only retrieved once per translation unit (because static). You can imagine to load a package that uses cpp11, then change the option to something funky, then load another package using cpp11 and have it crashed. I guess we could prepare a reprex to show that.

I certainly would not advise to change the option, but unfortunately there are not many allowed places where we can keep the external pointer.

@romainfrancois
Copy link
Collaborator

Instead of an option, maybe it can belong to an environment in the search path, e.g. similar to org:r-lib used by rlang. cc @lionel-

In an environment, it could perhaps be better protected.

@lionel-
Copy link
Member

lionel- commented Mar 22, 2022

I think it should be preserved in the namespace of the embedding package. This way all preserved objects are dominated by the package's namespace, making it much easier to debug leaks through tools like https://github.com/r-lib/memtools. Currently, the potentially leaking objects (not potentially in the case of lobstr, where there are actual leaks) are stored in a flat linked list in the precious list, making it hard to figure out what package is responsible for this memory.

I discussed this with Jim at one point, he said this setup should work for cpp11.

@HenrikBengtsson
Copy link
Author

Hi, thanks for the followup(s). My main use case that I thought of was situations like:

with_options({
  # something that triggers 'cpp11_preserve_xptr' to be set
  ...
})

which effectively will do options(cpp11_preserve_xptr = NULL) afterward.

@jimhester
Copy link
Member

For package use the cpp11 namespace won't exist, so the namespace of the package using cpp11 is probably the best option as Lionel suggested.

If you are using cpp11::source_cpp() and friends directly or via the cpp11 knitr engine there is no package namespace, but the cpp11 namespace would be loaded in those cases, so you could use that.

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.

4 participants