Fix for ALTREP crash in base R CHARSXP use-after-free

[traversc]

Here is a link to a reproducible example of the crash (tested on Linux and Mac) https://gist.github.com/traversc/4e04e761dc020bf4a47693e444bf73c4

The issue

vroom_chr::string_Elt() returns newly created CHARSXPs without keeping them reachable from the source ALTREP vector. A later allocation could therefore collect an element while base R code still held its pointer, which is a Use-After-Free.

I know that this was discussed before and is by design. So arguably this could be considered a base R bug but I think the fix is better placed in package because the fix:

1. also makes vroom faster and more efficient overall.
2. makes ALTREP and non-ALTREP strings semantically equal (so less footgun-y for Rcpp/cpp11 scripts)

The fix

The fix suggested by Claude (Fable) is to keep individually accessed strings in a STRSXP cache attached to the external pointer's "protected" slot. When the vector is materialized, the cache is promoted to data2 and only its missing elements are filled.

It looks like the "protected" slot R_SetExternalPtrProtected is exactly for this kind of thing, to keep R data alive that the external pointer needs.

This ends up being more efficient, because the materialization work for individual elements only happens once. You don't re-process individual CHARSXPs when you materialize the whole vector or access them again.

A previous issue: gagolews/stringi#354

[kevinushey]

Thanks for this, and for the detailed write-up and reproducer. I agree the crash needs fixing on our side: callers of STRING_ELT() are entitled to assume the result stays alive as long as the vector does, and R's own deferred-string ALTREP class solves it the same way (a full-length STRSXP allocated on the first Elt call and filled lazily). I built this branch alongside main: the new test fails on main and passes here, and the full suite passes.

I have a concern about memory I'd like to talk through, one correctness issue that needs fixing before merge, and a few smaller things.

Memory

Any element-wise pass over a lazy column (x == "a", %in%, nchar(), a for loop, ...) now retains every CHARSXP plus the cache vector, while vroom_str() still reports the column as not materialized. A single x[[i]] allocates the full 8-byte-per-row cache up front. On a 2M-row column of short strings:

	main	this PR
extra memory after x[[1]]	0 MB	15 MB
memory after one x == "s"	50 MB	202 MB
second x == "s"	0.14 s	0.018 s

I don't think the retention itself can be avoided: base R may hold pointers to many elements at once (unique(), match(), ...), so a bounded cache would just move the bug around. And vroom already holds an 8-byte offset per field in the index, so the cache vector is the same order as what we already pay. Still, going from 50 MB to 200 MB after a single comparison is a real change for people who lean on vroom's laziness, so before merging I'd like to:

1. Call this out in NEWS, since "lazy" now means "materialized on the first element-wise pass" rather than "never retained".
2. Hear whether you think it's worth avoiding the up-front allocation for the x[[i]] case, e.g. a small list of (index, CHARSXP) pairs for the first few dozen accesses, promoted to the full STRSXP after that. If you see a cheaper approach I'm all ears; if you think the complexity isn't worth it, say so and we can discuss.

problems() loses the correct row for embedded nuls

Val() reports the embedded-null error with info.column->begin(), so the row is always 1. That bug predates this PR, but it was masked: materializing re-parsed every element through read_chr(), which added the correct row too. Now fill_chr() skips cached elements, so the correct entry never gets added. With a file whose third row contains a nul byte, x[[3]] followed by problems() reports rows 1 and 3 on main but only row 1 on this branch.

The fix is one line, matching parse_value() in vroom_vec.h:

auto itr = col->begin() + i;

I checked that this makes problems() report row 3. A test for it would be great.

Smaller things

• fill_chr() duplicates the read_chr() loop. A fresh STRSXP is all R_BlankString, so read_chr() can become "allocate, then fill_chr()", and Materialize() can always go through EltCache() + fill_chr() rather than keeping two branches. While there, the loop counter should be R_xlen_t rather than auto i = 0.
• R_ExternalPtrProtected() returns R_NilValue rather than a C null, so the cache == nullptr checks are dead.
• NEWS: please add (@traversc, #638) and phrase it in terms of what a user sees, e.g. "Fixed a use-after-free crash when base R held an element of a lazy character column across an allocation".

[traversc]

On memory usage: your proposal seems reasonable enough. Two other ideas:

Chunking: store a VECSXP of length (n / 1024L + 1L). Each chunk starts as null and lazily initialized as a STRSXP of length 1024. This would be best when you iterate through the vector and still want to avoid the large up front allocation.

Hashmap stored in the struct. Similar to your idea, but instead of storing (index, CHARSXP) you can store a pairlist with growing STRSXP slices, then index into them from the hashmap. This would be best if you had a ton of random access.

There are lots of different approaches you could come up, but I don't think there is one approach that will be always better. So whichever approach you think is best I can update the PR with.

[kevinushey]

Thanks. Let's go with chunking: it covers the x[[i]] case without a separate small-list mode, and in the full-pass case it costs the same 8 bytes per element as the current PR.

I sketched it on top of this branch to make sure it holds up; the diff is at the bottom. The character tests and the full suite pass, and the same measurement as my earlier table gives:

2M rows of short strings	this PR	chunked
extra MB after x[[1]]	15.3	0.1
extra MB after x[[1]] and x[[n]]	15.3	0.1
extra MB after one x == "s"	152.5	152.6
second x == "s"	0.019 s	0.021 s
materialize after that pass	0.035 s	0.049 s

The shape:

• The cache is a VECSXP in the external pointer's protected slot holding 1024-element STRSXP chunks, allocated on first touch. R_BlankString still marks a miss within a chunk.
• Materialize() always goes through read_chr(info, cache), which is now "allocate, copy the cached chunks over, then fill_chr() the rest". That drops the duplicated loop and the two branches in Materialize(), at the cost of a pointer copy per cached element (the extra 14 ms on the last row above).
• I'd skip the hashmap. For the common element-wise passes (==, %in%, nchar()) it costs several times the memory of an array plus a hash lookup per element, and the sparse case it targets is already cheap with 8 KB chunks.
• I avoided std::min() on the static constexpr members: the package doesn't set CXX_STD, and under C++14 that would odr-use them.
• The new test touches the first, middle and last chunks, including empty and NA elements, then materializes and checks every value.

Feel free to take the diff as-is or rework it. The other items from my earlier comment still apply: the problems() row fix in Val() with a test, and the NEWS wording. The R_xlen_t counter and the dead nullptr checks fall out of the diff.

Diff against this branch

diff --git a/src/vroom_chr.cc b/src/vroom_chr.cc
index 44244705..90b5de4e 100644
--- a/src/vroom_chr.cc
+++ b/src/vroom_chr.cc
@@ -1,5 +1,7 @@
 #include "vroom_chr.h"
 
+static void copy_cached_chr(SEXP cache, SEXP out);
+
 SEXP check_na(SEXP na, SEXP val) {
   for (R_xlen_t i = 0; i < Rf_xlength(na); ++i) {
     SEXP v = STRING_ELT(na, i);
@@ -12,15 +14,26 @@ SEXP check_na(SEXP na, SEXP val) {
   return val;
 }
 
+cpp11::strings read_chr(vroom_vec_info* info, SEXP cache) {
+
+  R_xlen_t n = info->column->size();
+
+  cpp11::writable::strings out(n);
+
+  copy_cached_chr(cache, out);
+  fill_chr(info, out);
+
+  return out;
+}
+
 void fill_chr(vroom_vec_info* info, SEXP out) {
   SEXP nas = *info->na;
 
   cpp11::unwind_protect([&] {
-    auto i = 0;
+    R_xlen_t i = 0;
     auto col = info->column;
-    for (auto b = col->begin(), e = col->end(); b != e; ++b) {
+    for (auto b = col->begin(), e = col->end(); b != e; ++b, ++i) {
       if (STRING_ELT(out, i) != R_BlankString) {
-        ++i;
         continue;
       }
 
@@ -32,7 +45,7 @@ void fill_chr(vroom_vec_info* info, SEXP out) {
             b.index(), col->get_index(), "", "embedded null", b.filename());
       }
 
-      SET_STRING_ELT(out, i++, check_na(nas, val));
+      SET_STRING_ELT(out, i, check_na(nas, val));
       UNPROTECT(1);
     }
   });
@@ -40,34 +53,29 @@ void fill_chr(vroom_vec_info* info, SEXP out) {
   info->errors->warn_for_errors();
 }
 
-cpp11::strings read_chr(vroom_vec_info* info) {
-
-  R_xlen_t n = info->column->size();
-
-  cpp11::writable::strings out(n);
+// Copy strings cached by vroom_chr::string_Elt() into `out`, so that
+// materializing does not parse them a second time.
+static void copy_cached_chr(SEXP cache, SEXP out) {
+  if (cache == R_NilValue) {
+    return;
+  }
 
-  SEXP nas = *info->na;
+  R_xlen_t n_chunks = Rf_xlength(cache);
+  for (R_xlen_t c = 0; c < n_chunks; ++c) {
+    SEXP chunk = VECTOR_ELT(cache, c);
+    if (chunk == R_NilValue) {
+      continue;
+    }
 
-  cpp11::unwind_protect([&] {
-    auto i = 0;
-    auto col = info->column;
-    for (auto b = col->begin(), e = col->end(); b != e; ++b) {
-      auto str = *b;
-      auto val = info->locale->encoder_.makeSEXP(str.begin(), str.end(), true);
-      PROTECT(val);
-      if (Rf_xlength(val) < str.end() - str.begin()) {
-        info->errors->add_error(
-            b.index(), col->get_index(), "", "embedded null", b.filename());
+    R_xlen_t offset = c << vroom_chr::chunk_shift;
+    R_xlen_t len = Rf_xlength(chunk);
+    for (R_xlen_t j = 0; j < len; ++j) {
+      SEXP val = STRING_ELT(chunk, j);
+      if (val != R_BlankString) {
+        SET_STRING_ELT(out, offset + j, val);
       }
-
-      SET_STRING_ELT(out, i++, check_na(nas, val));
-      UNPROTECT(1);
     }
-  });
-
-  info->errors->warn_for_errors();
-
-  return out;
+  }
 }
 
 R_altrep_class_t vroom_chr::class_t;
diff --git a/src/vroom_chr.h b/src/vroom_chr.h
index 3f58f57c..96acba94 100644
--- a/src/vroom_chr.h
+++ b/src/vroom_chr.h
@@ -7,8 +7,13 @@
 #include "r_utils.h"
 #include "vroom_vec.h"
 
-cpp11::strings read_chr(vroom_vec_info* info);
+// Allocate and parse a full character vector for `info`, reusing any strings
+// already cached by vroom_chr::string_Elt() (see vroom_chr::EltChunk).
+cpp11::strings read_chr(vroom_vec_info* info, SEXP cache = R_NilValue);
+
+// Parse into `out` every element that is still R_BlankString.
 void fill_chr(vroom_vec_info* info, SEXP out);
+
 SEXP check_na(SEXP na, SEXP val);
 
 struct vroom_chr : vroom_vec {
@@ -16,6 +21,17 @@ struct vroom_chr : vroom_vec {
 public:
   static R_altrep_class_t class_t;
 
+  // Strings created by string_Elt() must stay reachable for as long as the
+  // vector does, because R may hold a pointer from STRING_ELT() across a
+  // later allocation. They are kept in fixed-size STRSXP chunks hanging off
+  // a VECSXP in the external pointer's protected slot. Chunks are allocated
+  // on first touch, so sparse access costs one chunk rather than a
+  // full-length vector. Within a chunk, R_BlankString marks a miss: empty
+  // strings share that permanent singleton and need no rooting.
+  static constexpr R_xlen_t chunk_shift = 10;
+  static constexpr R_xlen_t chunk_size = R_xlen_t(1) << chunk_shift;
+  static constexpr R_xlen_t chunk_mask = chunk_size - 1;
+
   // Make an altrep object of class `stdvec_double::class_t`
   static SEXP Make(vroom_vec_info* info) {
 
@@ -46,20 +62,6 @@ public:
 
   // ALTSTRING methods -----------------
 
-  // Keep strings created by string_Elt() reachable through data1. New
-  // STRSXPs contain R_BlankString, which serves as the cache miss marker.
-  // Empty strings share this permanent singleton and do not need rooting.
-  static SEXP EltCache(SEXP vec) {
-    SEXP ptr = R_altrep_data1(vec);
-    SEXP cache = R_ExternalPtrProtected(ptr);
-    if (cache == nullptr || TYPEOF(cache) != STRSXP) {
-      cache = PROTECT(Rf_allocVector(STRSXP, Length(vec)));
-      R_SetExternalPtrProtected(ptr, cache);
-      UNPROTECT(1);
-    }
-    return cache;
-  }
-
   static SEXP Val(SEXP vec, R_xlen_t i) {
     auto& info = Info(vec);
 
@@ -84,6 +86,33 @@ public:
     return val;
   }
 
+  // The cache chunk holding element `i`, allocating it (and the chunk
+  // vector) on first use.
+  static SEXP EltChunk(SEXP vec, R_xlen_t i) {
+    SEXP ptr = R_altrep_data1(vec);
+    SEXP chunks = R_ExternalPtrProtected(ptr);
+    if (chunks == R_NilValue) {
+      R_xlen_t n_chunks = (Length(vec) + chunk_mask) >> chunk_shift;
+      chunks = PROTECT(Rf_allocVector(VECSXP, n_chunks));
+      R_SetExternalPtrProtected(ptr, chunks);
+      UNPROTECT(1);
+    }
+
+    R_xlen_t c = i >> chunk_shift;
+    SEXP chunk = VECTOR_ELT(chunks, c);
+    if (chunk == R_NilValue) {
+      R_xlen_t len = Length(vec) - (c << chunk_shift);
+      if (len > chunk_size) {
+        len = chunk_size;
+      }
+      chunk = PROTECT(Rf_allocVector(STRSXP, len));
+      SET_VECTOR_ELT(chunks, c, chunk);
+      UNPROTECT(1);
+    }
+
+    return chunk;
+  }
+
   // the element at the index `i`
   //
   // this does not do bounds checking because that's expensive, so
@@ -95,15 +124,15 @@ public:
     }
     SPDLOG_TRACE("{0:x}: vroom_chr string_Elt {1}", (size_t)vec, i);
 
-    SEXP cache = EltCache(vec);
-    SEXP val = STRING_ELT(cache, i);
+    SEXP chunk = EltChunk(vec, i);
+    R_xlen_t j = i & chunk_mask;
+    SEXP val = STRING_ELT(chunk, j);
     if (val == R_BlankString) {
       val = PROTECT(Val(vec, i));
-      if (val != R_BlankString) {
-        SET_STRING_ELT(cache, i, val);
-      }
+      SET_STRING_ELT(chunk, j, val);
       UNPROTECT(1);
     }
+
     return val;
   }
 
@@ -116,21 +145,13 @@ public:
 
     SPDLOG_TRACE("{0:x}: vroom_chr Materialize", (size_t)vec);
     SEXP data1 = R_altrep_data1(vec);
-    SEXP cache = R_ExternalPtrProtected(data1);
-    if (cache != nullptr && TYPEOF(cache) == STRSXP) {
-      fill_chr(&Info(vec), cache);
-      R_set_altrep_data2(vec, cache);
-
-      // Once we have materialized we no longer need the cache or info
-      R_SetExternalPtrProtected(data1, R_NilValue);
-      Finalize(data1);
-
-      return cache;
-    }
-
-    auto out = read_chr(&Info(vec));
+    auto out = read_chr(&Info(vec), R_ExternalPtrProtected(data1));
     R_set_altrep_data2(vec, out);
+
+    // Once we have materialized we no longer need the cache or the info
+    R_SetExternalPtrProtected(data1, R_NilValue);
     Finalize(data1);
+
     return out;
   }
 
diff --git a/tests/testthat/test-chr.R b/tests/testthat/test-chr.R
index b70d6bc1..4269d0cc 100644
--- a/tests/testthat/test-chr.R
+++ b/tests/testthat/test-chr.R
@@ -67,3 +67,35 @@ test_that("ALTREP character elements stay alive across allocations", {
   expect_identical(first, NA_integer_)
   expect_identical(second, NA_integer_)
 })
+
+test_that("cached ALTREP character elements are reused when materializing", {
+  n <- 2500L
+  expected <- sprintf("v%04d", seq_len(n))
+  expected[c(3L, 1025L)] <- ""
+  expected[c(4L, 1026L)] <- NA_character_
+
+  path <- withr::local_tempfile()
+  # Quote the empty strings so that they are not skipped as empty rows.
+  lines <- ifelse(expected == "", '""', expected)
+  writeLines(ifelse(is.na(lines), "NA", lines), path)
+
+  x <- vroom(
+    path,
+    delim = ",",
+    col_names = FALSE,
+    col_types = "c",
+    na = "NA",
+    altrep = "chr",
+    num_threads = 1L,
+    progress = FALSE,
+    show_col_types = FALSE
+  )[[1L]]
+
+  # Touch the first, middle and last chunks, including the empty and NA
+  # elements, so that materializing has to merge cached and uncached chunks.
+  idx <- c(1L, 3L, 4L, 1024L, 1025L, 1026L, 2049L, n)
+  expect_identical(vapply(idx, function(i) x[[i]], ""), expected[idx])
+
+  vroom_materialize(list(x), replace = FALSE)
+  expect_identical(x, expected)
+})

[traversc]

Updated, using your patch directly, and then fixing the error reporting bug and adding a test for it. Let me know if there's anything else!

[kevinushey]

Thanks for this -- the approach looks right, and I've confirmed the new regression tests fail on the pre-PR build as expected.

One issue I'd like fixed before merging: string_Elt() can write to a collected chunk if Val() re-enters R code that materializes the same column.

Val() calls info.errors->warn_for_errors() before returning, and a warning runs arbitrary R code in calling handlers. If a handler materializes the column -- for example by calling problems() on the data frame, which is exactly what the parse-issue warning tells users to do -- then Materialize() executes R_SetExternalPtrProtected(data1, R_NilValue) while string_Elt() is still holding chunk. After that, nothing roots chunk, so a GC before the resumed SET_STRING_ELT(chunk, j, val) writes to a dead object:

withCallingHandlers(
  x$col[[k]], # element with an embedded null
  vroom_parse_issue = function(w) invisible(problems(x))
)

The simplest fix is to keep chunk protected across the Val() call; writing into an orphaned chunk is then harmless, since the materialized data2 takes over:

SEXP chunk = PROTECT(EltChunk(vec, i));
R_xlen_t j = i & chunk_mask;
SEXP val = STRING_ELT(chunk, j);
if (val == R_BlankString) {
  val = PROTECT(Val(vec, i));
  SET_STRING_ELT(chunk, j, val);
  UNPROTECT(1);
}
UNPROTECT(1);

return val;

(Alternatively, re-check R_altrep_data2(vec) after Val() returns and skip the cache write once it is set.)

[traversc]

Updated to protect the cache chunk across Val(), as suggested. I think that is cleaner than the alternative approach you suggested.

One wrinkle: current R disables GC for the entire ALTSTRING dispatch so as far as I can tell theoretically the PROTECT is not necessary. So I didn't add a test for it since it passes either way.

[kevinushey]

Thanks! The PR looks good to me. We'll pick this up and merge once we begin work on the next vroom CRAN release.

[traversc]

For reference, filed an issue for arrow apache/arrow#51198