Merge remote-tracking branch 'origin/master' into rchk-gha

NEWS.md

@@ -66,7 +66,7 @@
 
 12. data.table's `all.equal()` method now dispatches to each column's own `all.equal()` method as appropriate, [#4543](https://github.com/Rdatatable/data.table/issues/4543). Thanks @MichaelChirico for the report and fix. Note that this had two noteworthy changes to data.table's own test suite that might affect you: (1) comparisons of POSIXct columns compare absolute, not relative differences, meaning that millisecond-scale differences might trigger a "not equal" report that was hidden before; and (2) comparisons of integer64 columns could be totally wrong since they were being compared on the basis of their representation as doubles, not long integers. The former might be a matter of preference requiring you to specify a different `tolerance=`, while the latter was clearly a bug.
 
-13. `rbindlist` could lead to a protection stack overflow when applied to a list containing many nested lists exceeding the pointer protection stack size, [#4536](https://github.com/Rdatatable/data.table/issues/4536). Thanks to @ProfFancyPants for reporting, and Benjamin Schwendinger for the fix.
+13. `rbindlist` and `shift` could lead to a protection stack overflow when applied to a list containing many nested lists exceeding the pointer protection stack size, [#4536](https://github.com/Rdatatable/data.table/issues/4536). Thanks to @ProfFancyPants for reporting, and Benjamin Schwendinger (`rbindlist`) and @MichaelChirico (`shift`) for the fix.
 
 14. `fread(x, colClasses="POSIXct")` now also works for columns containing only NA values, [#6208](https://github.com/Rdatatable/data.table/issues/6208). Thanks to @markus-schaffer for the report, and Benjamin Schwendinger for the fix.
 

inst/tests/tests.Rraw

@@ -18748,3 +18748,7 @@ test(2270, options=c(datatable.optimize=1L), DT[, mean(b, 1), by=a], data.table(
 DT1 = data.table(a=1:2)
 DT2 = data.table(a=c(1, 1, 2, 2), b=1:4)
 test(2271, options=c(datatable.verbose=TRUE), copy(DT1)[DT2, on='a', v := 4], copy(DT1)[, v := 4], output="x.a.\nAssigning")
+
+# shift of many elements accumulated PROTECT() for the fill values instead of releasing as soon as possible. Spotted by rchk in #6257.
+l = as.list(seq_len(2e4))
+test(2272, shift(l), as.list(rep(NA_integer_, 2e4)))

src/shift.c

@@ -40,7 +40,7 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type)
     SEXP elem  = VECTOR_ELT(x, i);
     size_t size  = SIZEOF(elem);
     R_xlen_t xrows = xlength(elem);
-    SEXP thisfill = PROTECT(coerceAs(fill, elem, ScalarLogical(0))); nprotect++;  // #4865 use coerceAs for type coercion
+    SEXP thisfill = PROTECT(coerceAs(fill, elem, ScalarLogical(0)));  // #4865 use coerceAs for type coercion
     switch (TYPEOF(elem)) {
     case INTSXP: case LGLSXP: {
       const int ifill = INTEGER(thisfill)[0];
@@ -170,8 +170,9 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type)
     default :
       error(_("Type '%s' is not supported"), type2char(TYPEOF(elem)));
     }
+    UNPROTECT(1); // thisfill
   }
-  UNPROTECT(nprotect);
-  return isVectorAtomic(obj) && length(ans) == 1 ? VECTOR_ELT(ans, 0) : ans;
+  if (isVectorAtomic(obj) && length(ans) == 1) ans = VECTOR_ELT(ans, 0);
+  UNPROTECT(nprotect); // ans, x?
+  return ans;
 }
-