From b754e8035330420b39d4eb92e32dc9ed89ae6f80 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Sun, 21 Jul 2024 13:33:10 -0700 Subject: [PATCH] More careful memory management in shift() (#6258) * More careful PROTECT() usage in shift * ws * actually merits a test+NEWS * Link an issue --- NEWS.md | 2 +- inst/tests/tests.Rraw | 4 ++++ src/shift.c | 9 +++++---- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index f23859487..5898724d5 100644 --- a/NEWS.md +++ b/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. diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 020a4db3e..a407d7e94 100644 --- a/inst/tests/tests.Rraw +++ b/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))) diff --git a/src/shift.c b/src/shift.c index e3e4a2b82..77d246d97 100644 --- a/src/shift.c +++ b/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; } -