Skip to content

protect fresh proxy get() results in conversion and assignment operators - #1492

Open
kevinushey wants to merge 1 commit into
masterfrom
bugfix/proxy-conversion-gc
Open

protect fresh proxy get() results in conversion and assignment operators#1492
kevinushey wants to merge 1 commit into
masterfrom
bugfix/proxy-conversion-gc

Conversation

@kevinushey

Copy link
Copy Markdown
Contributor

Fixes #1491.

The proxy operator T() conversions and the proxy-to-proxy assignment operators passed the raw result of get() into allocating calls: as<T>() coercions on the conversion side, and set() implementations that allocate before capturing the value on the assignment side. When get() returns a freshly computed SEXP (an active binding or active RefClass field, a field access evaluated via $, an attribute expanded by Rf_getAttrib()), that object is referenced from nowhere the GC can see, and a collection inside the window frees it while still in use. See #1491 for the analysis, including why standard builds of recent R rarely surface this (internal protection in coerceVector() since R 4.1.0, and R_UnwindProtect() accidentally keeping eval results reachable via the continuation token).

Changes:

  • api/meat/proxy.h: Shield the get() result in all thirteen operator T() conversions and in FieldProxy::operator=(const FieldProxy&).
  • proxy/SlotProxy.h: same for const_SlotProxy::operator T() (defined inline) and SlotProxy::operator=(const SlotProxy&).
  • proxy/Binding.h, proxy/NamesProxy.h: same for the proxy-to-proxy assignments.
  • Binding::operator=(const Binding&) additionally had a latent compile error: its self-assignment guard *this != rhs is ambiguous (the template conversion operators make it match unrelated != overloads), so the operator failed to compile whenever it was actually instantiated. The guard now compares the environment and name directly; the new regression test is this operator's first instantiation.
  • Reviewed but intentionally not changed: AttributeProxy::operator=(const AttributeProxy&) (its set() shields immediately upon entry) and generic_name_proxy::operator T() in vector/proxy.h (its get() returns a parent-reachable element).

Verification:

  • On an ASan/UBSan build of R-devel (r90339) with structure checking, the repro from proxy conversion and assignment operators use unprotected fresh SEXPs #1491 fails with unprotected object (0x...) encountered (was ENVSXP) against current master and runs clean with this patch.
  • New regression tests exercise FieldProxy and Binding copies from actively-computed values under gctorture(); they pass on standard builds and guard the fix on protect-checking builds such as CRAN's ASan setup.

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests (checked locally with RunAllRcppTests=yes on R 4.6.1; --no-manual --no-vignettes, so the only flagged items were two vignette-packaging warnings from the local --no-build-vignettes build)
  • Preferably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

Proxy operator T() conversions and proxy-to-proxy assignments passed the
raw result of get() into allocating calls (as<T>() coercions, set()
implementations that allocate before capturing the value). When get()
returns a freshly computed SEXP -- an active binding or active RefClass
field, a field access evaluated via $, an attribute expanded by
Rf_getAttrib() -- that object is referenced from nowhere the garbage
collector can see, and a collection triggered inside the window frees it
while still in use. Reproducible via gctorture() on an R built with
structure checking; see #1491 for details and a reproducible example.

Shield the get() result at each such site. Also fix
Binding::operator=(const Binding&), whose self-assignment guard
('*this != rhs') failed to compile whenever the operator was actually
instantiated; the new regression test is its first instantiation.

Fixes #1491.
if( *this != rhs )
set( rhs.get() ) ;
// NB: '*this != rhs' previously used here would not compile
// when this operator was instantiated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd. Thanks for persisting here too.

@eddelbuettel eddelbuettel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The actual change is, as you said, somewhat mechanical.

It's good to get to these.

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 this pull request may close these issues.

proxy conversion and assignment operators use unprotected fresh SEXPs

2 participants