[BUG] Write barrier in setv/copyv

[SebKrantz]

@claude I encountered the following issue. Please propose a minimal fix:

setv() / copyv() bypass R's write barrier for STRSXP and VECSXP targets

Package: collapse 2.1.6 and 2.1.7 (2.1.7 checked line-by-line; setcopyv is
unchanged between them)
File: src/programming.c, SEXP setcopyv(...) at line 262
Platform observed: macOS 15 / arm64, R 4.6.0. Nothing platform-specific about
the mechanism.

Summary

The in-place store loops in setcopyv() write element pointers straight into the
target vector's backing array. For unboxed types (INTSXP, REALSXP, LGLSXP,
RAWSXP, CPLXSXP) that is correct. For STRSXP and VECSXP it skips R's
generational write barrier, so an old-generation target can end up holding an
unrecorded reference to a younger CHARSXP / list element. A subsequent
young-generation collection frees the value while it is still referenced, and the
slot dangles.

The code

The three store macros (lines 312, 319, 329) all assign through a raw pointer:

#define setcopyvLOOP(e)                                     \
if(invert) {                                                \
  for(int i = 0; i != n; ++i) if(px[i] != v) px[i] = e;     \
} else {                                                    \
  for(int i = 0; i != n; ++i) if(px[i] == v) px[i] = e;     \
}

#define setcopyvLOOPLVEC1                                     \
if(tv == INTSXP) {                                            \
  _Pragma("omp simd")                                         \
  for(int i = 0; i < lv; ++i) px[pv[i]-1] = r;                \
  ...

and px for the boxed cases is obtained as a plain SEXP *:

case STRSXP:                                    // line 414
{
  SEXP *restrict px = set ? SEXPPTR(x) : SEXPPTR(ans);
  ...
case VECSXP:                                    // line 441
{
  if(set && ALTREP(x)) error("cannot modify ALTREP list by reference");
  SEXP *restrict px = set ? SEXPPTR(x) : SEXPPTR(ans);

px[i] = r is exactly what SET_STRING_ELT / SET_VECTOR_ELT exist to wrap:
those call CHECK_OLD_TO_NEW(x, v), which records the old-to-new reference so a
young-generation collection knows the value is reachable. Writing the pointer
directly omits that.

The exposure is greatest for set = TRUE (i.e. setv()), where the target is a
pre-existing, quite possibly old-generation vector. For set = FALSE
(copyv()), ans is freshly allocated and the loops do not allocate, so it is
far less likely to matter in practice.

Incidental observation: the _Pragma("omp simd") on those loops is also
vectorising pointer stores for the boxed cases, which the barriered setters would
prevent.

Symptoms

Non-deterministic, and all three of these appeared in one run:

CHAR() can only be applied to a 'CHARSXP', not a 'character'
Value of SET_STRING_ELT() must be a 'CHARSXP' not a 'character'
'Rf_translateCharUTF8' must be called on a CHARSXP, but got 'character'

plus plain segfaults with no R-level error. The failing read is typically far
from the setv() call — commonly the next unique(), qF() or forder() that
touches the affected column. When it lands inside data.table's forder, the
truelength state is left dirty and every subsequent column errors with
Internal error: savetl_init checks failed, which obscures the origin.

Where it was hit

osmclass::osm_classify(), which stores into character columns through an index
vector:

setv(class_res[1:3], has_tag, list(cat, tag, ...))
setv(res_tag, has_tag, tag_value, vind1 = TRUE)

i.e. the setcopyvLOOPLVEC1 path with a STRSXP target.

In a global OpenStreetMap pipeline this killed 21 of 72 Geofabrik extracts —
Belgium, Italy, the United Kingdom, Germany, France, Japan, Russia, Canada and
all five US regions. Smaller extracts completed fine: a 122-extract run over
low- and middle-income countries never triggered it, which matches a
GC-generation-timing dependence rather than anything about the data.

After rebuilding collapse with the barrier restored, all 21 extracts processed
successfully, and a control extract that had always worked (Rwanda) produced
byte-identical output — so the fix repairs the failure without changing
results.

Reproducer

I could not reduce this to a small synthetic case. Four attempts failed to
trigger it: direct value-match setv() on an old-generation character vector
with a freshly allocated replacement; the same under gctorture(TRUE); the
index-vector (vind1 = TRUE) form matching osmclass's call; and
osm_classify() itself on 300k rows of synthetic OSM-shaped data. Explicit
gc() makes it harder to reproduce, since a full collection scans old
generations and therefore finds the unrecorded reference anyway — the failure
needs a young-generation collection.

What reproduces it reliably is the real workload: running
osmclass::osm_classify(points, osm_point_polygon_class_det) over the points
layer of a large, densely tagged Geofabrik extract (Belgium, ~860k classified
rows after filtering, is the smallest that failed for me), inside a process that
has already done substantial allocation. Under stock 2.1.7 it fails; with the
barrier restored it does not.

Given the mechanism is clear from the code, the absence of a minimal reproducer
is probably just a matter of how hard it is to arrange the required generation
layout on demand.

Suggested fix

Use the barriered setters in the STRSXP and VECSXP branches — SET_STRING_ELT
and SET_VECTOR_ELT in place of px[i] = ..., keeping the numeric branches
byte-for-byte unchanged. A patch doing exactly that is at
vendor/collapse-patched/collapse_2.1.6_setv_writebarrier.patch in this repo
(~40 lines, one file; applies cleanly to both 2.1.6 and 2.1.7).

It was validated against stock 2.1.6 across 14,759 differential cases
(numeric/raw result-sets md5-identical; character/factor/list identical()),
collapse's own testthat suite (13,766 pass / 0 fail, same tally as stock), and
adversarial fuzzing under gctorture. Credit for the diagnosis and patch goes to
the author of the GID-parallelized fork.

[claude]

Claude finished @SebKrantz's task in 3m 5s —— View job • claude/issue-876-20260827-2130 • Create PR ➔

---

Summary

• Read issue and locate setcopyv() in src/programming.c
• Confirm the write-barrier bypass in the STRSXP/VECSXP branches
• Apply minimal fix: use SET_STRING_ELT/SET_VECTOR_ELT instead of raw pointer stores for boxed types
• Verify numeric/raw/logical branches are untouched
• Sanity-check the change (see note below on build/test)
• Commit, push, and provide PR link

I confirmed the mechanism described in the issue: in src/programming.c, setcopyv()'s STRSXP/VECSXP branches obtained a raw SEXP * via the SEXPPTR() macro ((SEXP *)DATAPTR_RO(x)) and assigned through it directly (px[i] = e) via the shared setcopyvLOOP/setcopyvLOOPLVEC1/setcopyvLOOPLVEC macros. That skips CHECK_OLD_TO_NEW(), which SET_STRING_ELT()/SET_VECTOR_ELT() normally call to record an old-to-new generational reference — exactly the scenario in the issue.

Fix applied

In src/programming.c:

• Added barrier-safe macro variants (setcopyvLOOP_SEXP, setcopyvLOOPLVEC1_SEXP, setcopyvLOOPLVEC_SEXP) that take the setter function (SET_STRING_ELT / SET_VECTOR_ELT) as a parameter and write through it against the target vector (x when set = TRUE, ans when set = FALSE), instead of writing through a raw pointer.
• STRSXP and VECSXP cases now read via SEXPPTR_RO() (const, read-only — used only for value-comparisons in the STRSXP value-match path) and write via SET_STRING_ELT()/SET_VECTOR_ELT() respectively.
• The numeric/logical/real/raw branches (INTSXP, LGLSXP, REALSXP, RAWSXP) are untouched — they still use the original raw-pointer macros, since the write barrier only applies to boxed types.
• Dropped the _Pragma("omp simd") on the boxed-type index-write path, since SET_STRING_ELT/SET_VECTOR_ELT are function calls (with barrier-check side effects) and aren't safely vectorizable — consistent with the issue's incidental observation.
• Added a NEWS.md entry under the in-development 2.1.7 section.

Note on verification

This sandboxed environment doesn't have R installed and I don't have permission to install it or run R CMD build/R CMD check/devtools::test() here, so I was unable to compile or run the package's testthat suite (13,766 tests referenced in the issue) or reproduce the GC-timing-dependent crash directly. The change is a targeted, mechanical substitution (raw pointer write → barriered setter call) that mirrors the same fix pattern the issue reporter already validated externally (14,759 differential cases + full testthat pass + gctorture fuzzing on the standalone patch). I'd recommend running the full test suite and an R CMD check in CI before merging.

If you can grant Bash access to an R toolchain, I can go back and compile/test this directly.