Skip to content

Avoid frollapply() stack overflows and protect limits #3994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattdowle merged 5 commits into from
Dec 8, 2019
Merged

Avoid frollapply() stack overflows and protect limits #3994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7da20f1
Allocate `dx` and `inx` with `R_alloc()`
DavisVaughan Oct 25, 2019
1308712
Restructure vectorized `k` loop to unprotect after usage
DavisVaughan Oct 25, 2019
a01b669
frollapply new fixes, disabled tests to verify interactively
jangorecki Oct 25, 2019
804163c
news entry
jangorecki Oct 25, 2019
9607681
add ctb to desc file
jangorecki Oct 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ Authors@R: c(
person("Roy","Storey", role="ctb"),
person("Manish","Saraswat", role="ctb"),
person("Morgan","Jacob", role="ctb"),
person("Michael","Schubmehl", role="ctb"))
person("Michael","Schubmehl", role="ctb"),
person("Davis","Vaughan", role="ctb"))
Depends: R (>= 3.1.0)
Imports: methods
Suggests: bit64, curl, R.utils, knitr, xts, nanotime, zoo, yaml
Expand Down
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

## BUG FIXES

1. `frollapply` could segfault and exceed R's C protect limits, [#3993](https://github.com/Rdatatable/data.table/issues/3993). Thanks to @DavisVaughan for reporting and fixing.

## NOTES


Expand Down
6 changes: 6 additions & 0 deletions inst/tests/froll.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,12 @@ test(6010.005, frollmean(d, 3:4), frollapply(d, 3:4, mean))
d = rbind(d, list(NA,NA))
ans = list(c(NA,NA,1.5,2,1.5,2,2.5), c(NA,NA,NA,2,1,1.5,2), c(NA,NA,1.25,1.5,1.75,1.5,2), c(NA,NA,NA,1.5,1,1.25,1.5))
test(6010.006, frollapply(d, 3:4, function(x, ...) if (sum(x, ...)>5) min(x, ...) else max(x, ...), na.rm=TRUE), ans)
# segfault and protect limits #3993 - disabled by default due to high memory usage
if (FALSE) {
test(6010.007, frollapply(1, rep(1L, 1e5), identity), as.list(rep(1, 1e5)))
test(6010.008, frollapply(1, rep(1L, 1e6), identity), as.list(rep(1, 1e6)))
test(6010.009, frollapply(as.list(rep(1, 1e6)), 1, identity), as.list(rep(1, 1e6)))
}
#### corner cases from examples
test(6010.101, frollapply(1:5, 3, function(x) head(x, 2)), error="frollapply: results from provided FUN are not length 1")
f = function(x) {
Expand Down
27 changes: 14 additions & 13 deletions src/frollR.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,8 @@ SEXP frollapplyR(SEXP fun, SEXP obj, SEXP k, SEXP fill, SEXP align, SEXP rho) {
if (verbose)
Rprintf("%s: allocating memory for results %dx%d\n", __func__, nx, nk);
ans_t *dans = (ans_t *)R_alloc(nx*nk, sizeof(ans_t));
double* dx[nx];
uint64_t inx[nx];
double** dx = (double**)R_alloc(nx, sizeof(double*));
uint64_t* inx = (uint64_t*)R_alloc(nx, sizeof(uint64_t));
for (R_len_t i=0; i<nx; i++) {
inx[i] = xlength(VECTOR_ELT(x, i));
for (R_len_t j=0; j<nk; j++) {
Expand All @@ -307,21 +307,22 @@ SEXP frollapplyR(SEXP fun, SEXP obj, SEXP k, SEXP fill, SEXP align, SEXP rho) {
dx[i] = REAL(VECTOR_ELT(x, i));
}

double *dw[nk];
SEXP pw[nk], pc[nk];
// in the following loop we handle vectorized k argument
// for each k we need to allocate own width window object: pw
double* dw;
SEXP pw, pc;

// in the outer loop we handle vectorized k argument
// for each k we need to allocate a width window object: pw
// we also need to construct distinct R call pointing to that window
for (R_len_t j=0; j<nk; j++) {
pw[j] = PROTECT(allocVector(REALSXP, ik[j])); protecti++;
dw[j] = REAL(pw[j]);
pc[j] = PROTECT(LCONS(fun, LCONS(pw[j], LCONS(R_DotsSymbol, R_NilValue)))); protecti++;
}
pw = PROTECT(allocVector(REALSXP, ik[j]));
dw = REAL(pw);
pc = PROTECT(LCONS(fun, LCONS(pw, LCONS(R_DotsSymbol, R_NilValue))));

for (R_len_t i=0; i<nx; i++) {
for (R_len_t j=0; j<nk; j++) {
frollapply(dx[i], inx[i], dw[j], ik[j], &dans[i*nk+j], ialign, dfill, pc[j], rho, verbose);
for (R_len_t i=0; i<nx; i++) {
frollapply(dx[i], inx[i], dw, ik[j], &dans[i*nk+j], ialign, dfill, pc, rho, verbose);
}

UNPROTECT(2);
}

if (verbose)
Expand Down