-
Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-51198: [R] ALTREP crash in base R due to use-after-free - #51288
GH-51198: [R] ALTREP crash in base R due to use-after-free #51288thisisnic wants to merge 5 commits into
Conversation
thisisnic
commented
Sep 10, 2026
@jonkeane - I had Claude walk me though this but the explanation below is all mine; tried to give a more understandable explanation of what's going on here. I feel I should probably just ditch the test instead of including one which calls gctorture() but figured leaving it here for a moment while we discuss this gives us a bit more context here.
Bug context
My understanding of this is that this is a bug which could trigger in specific occasions, where we have a string column that's been read in from a Parquet file or other Arrow -> R conversion path, and is being represented by altrep.
When Arrow creates R strings we can end up with a situation where they're left floating around in memory with no pointer, and the garbage collector gets rid of it even though R still needs it. This only affects unmaterialised values.
It happens in extremely specific circumstances: when an R function accesses an element of this column and then allocates memory before it has finished using the value. The allocation triggers the garbage collector, which frees the string, and we end up with a segfault or incorrect value.
This solution
Code
We basically create a cache that is attached to the ALTREP vector which lists the pointers of all the strings which have been accessed. We only add stuff to the cache when it's accessed. We do this lazily in chunks like the vroom PR which is linked to by the issue author.
🤖 told me to add: "When the column is later materialised, cached strings are copied into the new vector instead of converted again so they stay alive after the cache is dropped"
Tests
The test here uses gctorture() which makes the garbage collector run on every single allocation, otherwise we wouldn't see the error (n.b. the garbage collector only usually runs when when memory crosses a threshold).
I ran the test without the fix and then with the fix, and got a segfault and then no segfault.
I had a minor concern is that this is a bug that only shows up in extremely specific circumstances, which may be rare, but the fix increases memory footprint for elements that have been accessed. But no worse than not using altrep at all.
1bf39d5 to
e6b5ba1
Compare
thisisnic
commented
Sep 10, 2026
I also had Claude run some local benchmarks to test the speed difference; will get it to post a summary in another comment.
thisisnic
commented
Sep 10, 2026
|
This comment was written by Claude (an AI assistant) at @thisisnic's request, not by Nic. Nic ran the session; the benchmark design, numbers, and reading below are mine. I benchmarked the cache against the same code with
Reading it:
So the cost is memory rather than time, only for elements that have actually been accessed, and never more than not using ALTREP at all. Benchmark scriptsuppressMessages(devtools::load_all(quiet = TRUE)) options(arrow.use_altrep = TRUE) library(bench) n <- 1e6L strings <- sprintf("string number %07d", seq_len(n)) arr <- Array$create(strings) fresh <- function() { x <- arr$as_vector() stopifnot(is_arrow_altrep(x), !test_arrow_altrep_is_materialized(x)) x } set.seed(1) sparse_idx <- sample.int(n, 1000L) bench::mark( sparse_access = { x <- fresh(); for (i in sparse_idx) x[[i]] }, repeat_access = { x <- fresh(); for (k in 1:10) for (i in sparse_idx) x[[i]] }, full_pass = { x <- fresh(); for (s in x) NULL }, materialize_fresh = { x <- fresh(); test_arrow_altrep_force_materialize(x) }, pass_then_materialize = { x <- fresh(); for (s in x) NULL; test_arrow_altrep_force_materialize(x) }, iterations = 5, check = FALSE, memory = TRUE ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It changes GC/lifetime behavior at the R ALTREP boundary, which warrants careful human validation across R versions and build/check environments.
Pull request overview
Fixes an R ALTREP use-after-free / segfault scenario by ensuring Elt()-produced CHARSXP values remain reachable across garbage collection until the ALTREP vector is materialized, aligning Arrow’s ALTREP string behavior with base R’s expectations.
Changes:
- Add a block-based CHARSXP cache for ALTREP string vectors and reuse it during materialization (
r/src/altrep.cpp). - Add regression tests for GC-survival of
Elt()results and for cache reuse on materialization (r/tests/testthat/test-altrep.R). - Document the fix and its potential memory impact in R NEWS (
r/NEWS.md).
File summaries
| File | Description |
|---|---|
| r/src/altrep.cpp | Introduces a protected-slot cache for Elt() strings and copies cached values into the materialized STRSXP to avoid GC invalidation. |
| r/tests/testthat/test-altrep.R | Adds regression coverage for GC safety of Elt() and verifies materialization reuses cached strings. |
| r/NEWS.md | Notes the crash fix and calls out potential increased memory use for element-wise access. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Changes recommended
The new code introduces a potential compilation issue (std::min without a guaranteed <algorithm> include), which should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
It changes low-level ALTREP/GC interaction in C++ where subtle protection/lifetime mistakes can reintroduce crashes, so it warrants final human review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
thisisnic
commented
Sep 10, 2026
@github-actions crossbow submit -g r
|
Revision: 5060cc5 Submitted crossbow builds: ursacomputing/crossbow @ actions-f50c6869b7
|
Uh oh!
There was an error while loading. Please reload this page.
Rationale for this change
Segfault triggered in certain circumstances when creating an ALTREP string column and garbage collection runs
What changes are included in this PR?
Cache string ALTREP value pointer to prevent problematic reallocation after garbage collection
Are these changes tested?
Yeah
Are there any user-facing changes?
Increased memory usage in some circumstances but see benchmarks below