segfault: terminate called after throwing an instance of 'Rcpp::eval_error'

[mtmorgan]

I'm not really sure where the responsibility for this lies, but

suppressPackageStartupMessages({
    library(conflicted)
    library(BiocGenerics)  # not too heavy, exports a tweaked evalq
    library(xml2)
})
read_html("https://bioconductor.org")

results in a segfault

terminate called after throwing an instance of 'Rcpp::eval_error'
  what():  Evaluation error: evalq found in 2 packages. You must indicate which one you want with ::
 * BiocGenerics::evalq
 * base::evalq.

Normally (e.g., typing evalq at the command line), 'conflicted' would report the conflict between the two definitions of evalq as an error.

Rcpp seems to run in to trouble at inst/include/Rcpp/api/meat/Rcpp_eval.h:118 and 134

    Shield<SEXP> res(internal::Rcpp_eval_impl(call, R_GlobalEnv));
    ...
            throw eval_error(CHAR(STRING_ELT(conditionMessage, 0)));

the call stack before the segfault is

(gdb) where
#0  Rcpp::Rcpp_eval (expr=0x150b520, env=0x64b9d8)
    at ../inst/include/Rcpp/api/meat/Rcpp_eval.h:98
#1  0x00007fffea3fb5c3 in get_rcpp_cache () at barrier.cpp:100
#2  0x00007fffea3fb6ac in rcpp_get_stack_trace () at barrier.cpp:119
#3  0x00007fffea107834 in rcpp_get_stack_trace ()
    at /home/mtmorgan/R/x86_64-pc-linux-gnu-library/3.5-Bioc-3.8/Rcpp/include/Rcpp/routines.h:125
#4  0x00007fffea108b4d in exception_to_r_condition (ex=...)
    at /home/mtmorgan/R/x86_64-pc-linux-gnu-library/3.5-Bioc-3.8/Rcpp/include/Rcpp/exceptions.h:303
#5  0x00007fffea0f7752 in _xml2_read_connection_ (conSEXP=0x12bd038, 
    chunk_sizeSEXP=0x12bd2d8) at RcppExports.cpp:19
#6  0x00007ffff7816a59 in R_doDotCall (
    ofun=0x7fffea0f7491 <_xml2_read_connection_(SEXP, SEXP)>, nargs=2, 
    cargs=0x7ffffffdec20, call=0x974c18)
    at /home/mtmorgan/src/R-3-5-branch/src/main/dotcode.c:573
...

and the expression leading to the exception is, after stepping to line 118

(gdb) call Rf_PrintValue(call->t)
tryCatch(evalq(getNamespace("Rcpp"), <environment>), error = function (x) 
x, interrupt = function (x) 
x)

I guess the problem is that an exception occurs while Rcpp is trying to handle an exception?

This is the third time through Rcpp_eval(); the second time through generates the 'conflicted' error. The code in xml2 is

  Rcpp::Environment baseEnv = Rcpp::Environment::base_env();
  Rcpp::Function readBin = baseEnv["readBin"];

  RawVector out = Rcpp::as<RawVector>(readBin(con, "raw", bytes));

where somehow it seems like evalq() used to evaluate readBin() should be the version seen by the xml2 namespace rather than found on the search path?

The context is at r-lib/conflicted#8

[eddelbuettel]

Well if it works without the new conflicted package I would say: don't use the conflicted package here.

We do have other pending patches for eval and exceptions so maybe @lionel- wants to take a peek if this is better in his branch.

[mtmorgan]

My understanding is that conflicted will work around this, but there's nothing like fixing the problem at the source. Here's the Rcpp-only error

library(Rcpp)
fun <- cppFunction("
    NumericVector fun(Function fun1) {
        return fun1(1);
    }")
fun(sqrt)
evalq <- function(...) stop()  # I'm naive, what do I know about evalq?
fun(sqrt)

[lionel-]

I will look into it.

[lionel-]

And thanks for the reprex @mtmorgan!

[eddelbuettel]

I don't see where the reprex qualifies as an Rcpp issue after you deliberate muck with the evaluation model. You break, get to keep the pieces.

edd@rob:/tmp$ cat worksforme.R 
library(Rcpp)
fun <- cppFunction("
    NumericVector fun(Function fun1) {
        return fun1();
    }")
fun(sqrt)
cat("works for me\n")
edd@rob:/tmp$ Rscript worksforme.R 
Error in fun(sqrt) : 
  Evaluation error: 0 arguments passed to 'sqrt' which requires 1.
Calls: fun -> .Call
Execution halted
edd@rob:/tmp$ 

We supply matches. If you play with fire and burn yourself, is that really our fault?

[lionel-]

The correct assessment is that Rcpp is currently broken. It should be properly encapsulated just like any namespaced code but it isn't right now.

[mtmorgan]

If as a user I were to write

fun = function(fun1) evalq(fun1(1))
evalq = function(...) { ... }

I'd get burned. I'd then revise my function to

fun = function(fun1) base::evalq(fun1(1))

and be able to play with matches (but maybe evalq() isn't meant to be a match, just a poorly named function that does something unrelated to the language) and not get burned.

If I were writing a package, I wouldn't have to base::evalq() because the namespace mechanism would protect me from whatever the user put on the search path. But I could do that, if I were a suspenders-and-belt kind of guy. And if I were implementing a function from a non-base package and strictly for internal use, I'd protect myself from users putting arbitrary things on the search path by importing the symbols I intend to use.

How are these use cases different from what Rcpp does, but at the C level? Rcpp intends to use base::evalq(), but gets stuck with whatever is on the search path.

I think I'm not putting words in your mouth to say that we all strive to minimize unexpected terminations, viewing these as serious bugs even in the face of user error (e.g., we'd be really disappointed if sqrt(letters) caused a segfault).

Note that we also have other issues with the Rcpp implementation of fun, like

> tryCatch <- function(...) "baseball is fun"
> fun(sqrt)

 *** caught segfault ***
address 0x1, cause 'memory not mapped'

I'm only persisting here because I'd like to see Lionel's patch / efforts realized; if we're just talking amongst friends then I'll carry on over drinks with you sometime in the future.

[eddelbuettel]

Sorry, but now you bring the pending patch in. Did you try this problem under it? Is it resolved there? If so, nobody told me yet.

[mtmorgan]

Sorry bad communication on my part. I meant to convey that I was anticipating that Lionel would address the issue, if not discouraged by you. I don't know of any existing solution, the above is from

> packageVersion("Rcpp")
[1] '0.12.17'

installed from github master.

[eddelbuettel]

Well we have been trying to get exceptions, and stop(), and related function "right" for several years now and I think we are pretty close now. I like that.

Everything else we brought up here so far seems like a somewhat marginal corner case to me. Nice to have. not the core functionality we fixed. I would not want to risk to the latter for the former.

But well written patches and pull requests are always welcome. I would also welcome solid tests on those, preferably those that are complementary to what I do (near-full rev.dep checks on Linux). So I suggested to Lionel to maybe looking into using Docker / Rocker to test on older R versions. I have not yet heard back from him on that.

[kevinushey]

You can see the current (not yet @lionel- version of unwind-protect) version of Rcpp_eval here:

Rcpp/inst/include/Rcpp/api/meat/Rcpp_eval.h

Lines 100 to 113 in 6886a43

	// 'identity' function used to capture errors, interrupts
	SEXP identity = Rf_findFun(::Rf_install("identity"), R_BaseNamespace);
	if (identity == R_UnboundValue) {
	stop("Failed to find 'base::identity()'");
	}
	// define the evalq call -- the actual R evaluation we want to execute
	Shield<SEXP> evalqCall(Rf_lang3(::Rf_install("evalq"), expr, env));
	// define the call -- enclose with `tryCatch` so we can record and forward error messages
	Shield<SEXP> call(Rf_lang4(::Rf_install("tryCatch"), evalqCall, identity, identity));
	SET_TAG(CDDR(call), ::Rf_install("error"));
	SET_TAG(CDDR(CDR(call)), ::Rf_install("interrupt"));

Note that the intention here is effectively to construct a call of the form:

tryCatch(evalq(<expr>), error = identity, interrupt = identity)

and evaluate that in the requested environment. If you provide alternate definitions for tryCatch, evalq or identity in that namespace those get picked up and you get the unexpected result.

I think we would accept a PR that modified this to instead appear as:

base::tryCatch(base::evalq(<expr>), condition = base::identity)

Note that this would imply needing to recompile any packages using Rcpp as well to ensure they pick up the new definition of Rcpp_eval after the fix lands.

[kevinushey]

That said, the easiest solution here is just "don't load conflicted", so ...

[mtmorgan]

I'm not in a position (i.e., I do not have enough experience with the Rcpp internals or indeed C++, especially not at the level of sophistication of Rcpp) to write Rcpp code, so hopefully the pull request will come from elsewhere in the community.

I really don't understand the 'don't load conflicted'; we've seen that the problem is easily tickled using plain old R code in conjunction with any package that uses Rcpp's Function() (see, I don't even know how to reference this usage!).

[dselivanov]

Same story as with !! - invent something weird and make everyone suffer.

[kevinushey]

You do still need to do something 'strange' -- I think it's relatively uncommon that users might define their own versions of tryCatch or evalq that do something 'weird'; conflicted seems to be the first package that hijacks its implementation here in such a way. There's a reason why this is the first time we've heard about this issue, and this code has been live in Rcpp for quite some time.

I agree that the issue is real, but it's ultimately rare and is only triggered by packages or users doing funky things.

[lionel-]

@kevinushey I think the fix is just to evaluate the tryCatch(evalq(...)) call in the base namespace rather than the global environment, but I haven't tried it yet.

[lionel-]

s/namespace/package since the former inherits from the search path (just to be on the safe side).

[JoFAM]

@lionel- not sure about that. Make sure you test with Bioconductor as well. Many packages there depend on Rcpp and on the redefined eval and evalq in BiocGenerics. I expect more issues with Bioconductor given how conflicted uses a very odd way of messing with the search path (and silently masking library and require).

[kevinushey]

Would we ever want Rcpp_eval to use the version of evalq defined by BiocGenerics? (seems like this shouldn't be the case, but it's worth asking)

[lionel-]

@JoFAM We definitely should use base::evalq().

[mtmorgan]

I'll just report that @kevinushey 's patch fixes both my simpler examples and the original segfault in r-lib/conflicted#8 ; this did require recompilation of all packages using Rcpp. Thanks!

[eddelbuettel]

@mtmorgan Cool, thanks for reporting back!

To be plain, when you say "patch" you are referring to the new branch that is not yet a PR?

[mtmorgan]

Yes, this branch

Rcpp bugfix/rcpp-eval-base-symbols$ git branch --list
* bugfix/rcpp-eval-base-symbols
  master

[JoFAM]

@lionel- using base::evalq seems the most logical thing to do. I'm just pointing out that this should be at least communicated to the Bioconductor team (edit: I mean the Bioconductor community/package devs, apologies ) as well, as there is a possibility of things going awry. eval in biocGenerics is a S4 generic that dispatches to base::eval in most cases. But not in all.

Notified bioc-devel : https://www.mail-archive.com/bioc-devel@r-project.org/msg09813.html

[lionel-]

There is no possibility of things going awry. This is private Rcpp code that should be namespaced. This is exactly the same situation as any package code except we have to evaluate R code manually from C++ and doing it in the global env rather than in Rcpp's namespace (or equivalently the base env since we only use base functions here) is a bug.

[lionel-]

On second thought you're right this could have consequences for bioc packages under a particular set of circumstances:

• They added BioGenerics to Depends: so that BioGenerics::evalq() is always on the search path
• They rely on Rcpp_eval() to dispatch on arguments.

Sorry for unduly dismissing your remarks.

Edit: Such packages would only work correctly if they are attached with a library() call or if BioGenerics is attached. Calling them with :: would produce unreliable results. So if there are any package relying on this behaviour, it's a good thing they will be forced to explicitly call into Biogenerics::eval() in their Rcpp_eval() calls.

[mtmorgan]

This sounds (from outside the Rcpp universe) almost like using an internal Rcpp function rather than the public Rcpp interface. If there is some breakage (there are no calls to Rcpp_eval() directly in the Bioc code base, how else might the user end up at Rcpp_eval?), it seems like the author should be doing the equivalent of BiocGenerics::evalq() if that's what they require for dispatch. I'm more than willing to live with this fallout.

[JoFAM]

@mtmorgan I wasn't clear enough in my previous postings. I obviously don't encourage using undocumented functions/ relying on unintended behaviour, but I have seen packages before that do things in non-standard ways. I merely wanted the people who have packages in the Bioconductor system to be aware, and wondered if there were contributed packages that did such a thing. Hence my suggestion and mail on bioc-devel.

To be clear: even though it might sound that way, it was not my intent to suggest that the official packages of the Bioconductor core team would do such a thing. My apologies for possible confusion, I hope I clarified myself.

[lionel-]

there are no calls to Rcpp_eval() directly in the Bioc code base, how else might the user end up at Rcpp_eval?

It's part of the Rcpp API so it is public. I agree it's ok (and even desirable) to break packages relying on these semantics, if they exist.

[eddelbuettel]

Fixed via #863