r-lib / cpp11

cpp11 helps you to interact with R objects using C++ code.
https://cpp11.r-lib.org/
Other
193 stars 46 forks source link

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

Closed HenrikBengtsson closed 11 months ago

HenrikBengtsson commented 2 years ago

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

https://github.com/r-lib/cpp11/blob/9a62c3a420b076d6ad48c291039421ff739dcc58/inst/include/cpp11/protect.hpp#L380-L405

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 commented 2 years ago

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 commented 2 years ago

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- commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.