Closes #1014. Similar to 'shift()' issue in #1009.

Rdatatable · Jan 21, 2015 · f952ba1 · f952ba1
1 parent 1246762
commit f952ba1
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 6 deletions.
diff --git a/R/shift.R b/R/shift.R
@@ -1,6 +1,6 @@
 shift <- function(x, n=1L, fill=NA, type=c("lag", "lead"), give.names=FALSE) {
     type = match.arg(type)
-    ans = .Call("Cshift", x, as.integer(n), fill, type)
+    ans = .Call(Cshift, x, as.integer(n), fill, type)
     if (give.names) {
         if (is.null(names(x))) 
             nx = paste("V", if (is.atomic(x)) 1L else seq_along(x), sep="")

diff --git a/README.md b/README.md
@@ -107,7 +107,7 @@
 
   28. Overlapping range joins with `-Inf` and `Inf` and 0.0 in them are handled properly now. Closes [#1006](https://github.com/Rdatatable/data.table/issues/1006). Thanks to @tdhock for filing the issue with a nice reproducible example.
 
-  29. Fixed a segfault in `shift()` when number of rows in `x` is lesser than value for `n`. Closes [#1009](https://github.com/Rdatatable/data.table/issues/1009). Thanks to @jangorecki for the reproducible report.
+  29. Fixed two segfaults in `shift()` when number of rows in `x` is lesser than value for `n`. Closes [#1009](https://github.com/Rdatatable/data.table/issues/1009) and [#1014](https://github.com/Rdatatable/data.table/issues/1014). Thanks to @jangorecki and @ashinm for the reproducible reports.
 
 #### NOTES
 

diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw
@@ -5798,6 +5798,8 @@ dt.ref <- data.table(start=x[-n], end=x[-1], key=c("start", "end"))
 dt.query <- data.table(q1=c(-0.2, -0.05, 0.05, 0.15), q2=c(-0.2, -0.05, 0.05, 0.15), key=c("q1", "q2"))
 test(1471, foverlaps(dt.query, dt.ref), data.table(dt.ref, dt.query, key=c("q1", "q2")))
 
+# #1014 (segfault) fix
+test(1472, shift(1, 1:2, NA, 'lag'), list(NA_real_, NA_real_))
 
 ##########################
 

diff --git a/src/shift.c b/src/shift.c
@@ -42,13 +42,14 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {
                 case INTSXP : 
                     thisfill = PROTECT(coerceVector(fill, INTSXP));
                     for (j=0; j<nk; j++) {
+                        thisk = (xrows >= INTEGER(k)[j]) ? INTEGER(k)[j] : xrows;
                         tmp = allocVector(INTSXP, xrows);
                         SET_VECTOR_ELT(ans, i*nk+j, tmp);
                         if (xrows - INTEGER(k)[j] > 0)
                             memcpy((char *)DATAPTR(tmp)+(INTEGER(k)[j]*size), 
                                    (char *)DATAPTR(this), 
                                    (xrows-INTEGER(k)[j])*size);
-                        for (m=0; m<INTEGER(k)[j]; m++)
+                        for (m=0; m<thisk; m++)
                             INTEGER(tmp)[m] = INTEGER(thisfill)[0];
                         copyMostAttrib(this, tmp);
                         if (isFactor(this))
@@ -68,14 +69,17 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {
                         thisfill = PROTECT(coerceVector(fill, REALSXP));
                     }
                     for (j=0; j<nk; j++) {
+                        thisk = (xrows >= INTEGER(k)[j]) ? INTEGER(k)[j] : xrows;
                         tmp = allocVector(REALSXP, xrows);
                         SET_VECTOR_ELT(ans, i*nk+j, tmp);
-                        if (xrows - INTEGER(k)[j] > 0)
+                        if (xrows - INTEGER(k)[j] > 0) {
                             memcpy((char *)DATAPTR(tmp)+(INTEGER(k)[j]*size), 
                                    (char *)DATAPTR(this), 
                                    (xrows-INTEGER(k)[j])*size);
-                        for (m=0; m<INTEGER(k)[j]; m++)
+                        }
+                        for (m=0; m<thisk; m++) {
                             REAL(tmp)[m] = REAL(thisfill)[0];
+                        }
                         copyMostAttrib(this, tmp);
                         if (isFactor(this))
                             setAttrib(tmp, R_LevelsSymbol, getAttrib(this, R_LevelsSymbol));
@@ -85,13 +89,14 @@ SEXP shift(SEXP obj, SEXP k, SEXP fill, SEXP type) {
                 case LGLSXP :
                     thisfill = PROTECT(coerceVector(fill, LGLSXP));
                     for (j=0; j<nk; j++) {
+                        thisk = (xrows >= INTEGER(k)[j]) ? INTEGER(k)[j] : xrows;
                         tmp = allocVector(LGLSXP, xrows);
                         SET_VECTOR_ELT(ans, i*nk+j, tmp);
                         if (xrows - INTEGER(k)[j] > 0)
                             memcpy((char *)DATAPTR(tmp)+(INTEGER(k)[j]*size), 
                                    (char *)DATAPTR(this), 
                                    (xrows-INTEGER(k)[j])*size);
-                        for (m=0; m<INTEGER(k)[j]; m++)
+                        for (m=0; m<thisk; m++)
                             LOGICAL(tmp)[m] = LOGICAL(thisfill)[0];
                         copyMostAttrib(this, tmp);
                         if (isFactor(this))