Clear external pointer before calling finalizer

[krlmlr]

Closes r-dbi/RPostgres#167.

Apologies for the terse pull request, I've been chasing the reason for the downstream problem for some time now.

The issue manifests itself in segmentation faults that depend on the registration order of finalizers. When the external pointer is cleared too late (i.e. after the Rcpp finalizer is run), a situation arises where my code receives an XPtr that claims to be valid but where the containing object already has been deleted.

The PR at hand fixes the downstream problem. Happy to provide more information as needed.

[eddelbuettel]

You may have noticed that we added something to the repo to justify / clarify PRs, especially when they come without prior issue ticket discussion.

Yet you completely ignored this.

[eddelbuettel]

And without context, it is not completely clear why nulling a pointer before calling its finalizer is a good idea. See WRE Sec 5.13.1 -- showing they clearly call as part of the finalizer.

Edit: Missed the passing from p to ptr on first read.

[krlmlr]

Apologies for the terse pull request, I've been chasing the reason for the downstream problem for some time now.

I think the example in "Writing R Extensions 5.13.1" would exhibit the same behavior if tortured enough, in particular if the finalizer calls code that uses the external pointer that is currently finalized.

This patch establishes a clear order:

1. Get the payload of the external pointer.
2. Clear the external pointer.
3. Finalize the payload.

So, the XPtr object becomes "empty" before finalization of the payload starts, and any code that somehow gets hold of this XPtr object can no longer access the payload that is now in the process of destruction.

[kevinushey]

This PR looks good to me.

One minor concern: in theory, a user-provided finalizer might still want to access R_ExternalPtrAddr() through the EXTPTR SEXP itself; however, I'm guessing hardly anyone provides their own custom finalizers for XPtr, and anyone who does probably wouldn't do this (since they already have access to the payload, as you said).

As an aside, the pattern shown in the R-ext examples uses code of the form:

static void finalizer(SEXP ptr)
{
    if (!R_ExternalPtrAddr(ptr))
        return;

    /* do stuff */

    R_ClearExternalPtr(ptr);
}

Would it be sufficient for us to explicitly check R_ExternalPtrAddr(ptr) != NULL to ensure that the pointer hasn't already been finalized? That check could be made here:

https://github.com/RcppCore/Rcpp/pull/1038/files#diff-13b1b8cc1aca82d80a21e53b89773fd3R37

If that works, I would prefer that solution simply because it's more in-line with the R documentation.

It's also possible that this is a safety check worth making regardless, but your PR remains necessary to fix the underlying issue.

[eddelbuettel]

I had a similar thought reading WRE 5.13.1. I can add it after the fact.

About 1/3 with reverse depends checks. No smoking gun yet.

[krlmlr]

@kevinushey: In my code I'm seeing that an already deleted pointer is used and available via the XPtr. I tried https://github.com/krlmlr/Rcpp/commit/3ef42164583fb58935e6bf3be02d2580c4eb1f86 and this didn't fix my downstream problem. Tried this PR again to double-check that this does fix my downstream problem -- it does.

Would you like me to add something similar on top of this PR, or after merging, to protect against double finalization?

[eddelbuettel]

Those are not complete alternatives. There is no reason to not test for XPtr and NULL before adding the clearing call you suggest. Something like

template <typename T, void Finalizer(T*) >
void finalizer_wrapper(SEXP p) {
    if (TYPEOF(p) != EXTPTRSXP)
        return;

    T* ptr = (T*) R_ExternalPtrAddr(p);
    RCPP_DEBUG_3("finalizer_wrapper<%s>(SEXP p = <%p>). ptr = %p", DEMANGLE(T), p, ptr)

    if (ptr == NULL)
        return;

    // Clear before finalizing to avoid behavior like access of freed memory
    R_ClearExternalPtr(p);

    Finalizer(ptr);
}

[kevinushey]

Sounds good. It sounds like your fix is indeed necessary. (and thanks to @eddelbuettel for bringing in my suggestion as well)

Looks good to me -- thanks for taking the time to investigate and put this together!

[krlmlr]

You're welcome, thanks for reviewing!

[eddelbuettel]

Two pretty clean reverse depends checks:

• RcppCore/rcpp-logs@a19c041
• RcppCore/rcpp-logs@74bec01