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.