joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
219 stars 71 forks source link

Commit 02300bdc9375c868bc31debe6f5d748cd03aa391 introduces a segfault #234

Closed TomAndrews closed 6 years ago

TomAndrews commented 6 years ago

Description

Commit 02300bdc9375c868bc31debe6f5d748cd03aa391 seems to introduce a segfault in my code.

I'm afraid I haven't managed to create a reproducible example, let alone a minimal one, but running various jobs before and after this commit point to it being the culprit.

I think the problem is here: https://github.com/joshuaulrich/xts/blob/master/src/na.c#L274 If the entire final column is NAs then I think _first will be the length of the whole SEXP [i.e. the last valid index + 1] and so we write off the end of real_result when copying the NAs below.

I will create a PR shortly with what seems to fix my problem.

Thanks for all the good work!

joshuaulrich commented 6 years ago

Thanks for the report!

Sometimes you can force memory issues like this to surface using gctorture(). Other times you have to use valgrind or a sanitizer. In this case, gctorture() works:

x <- xts::.xts(matrix(c(NA, 1, NA, NA), 2, 2), 1:2)
zoo::na.locf(x)
#                     [,1] [,2]
# 1969-12-31 18:00:01   NA   NA
# 1969-12-31 18:00:02    1   NA
gctorture()
xts:::na.locf.xts(x)

 *** caught segfault ***
address (nil), cause 'unknown'
Segmentation fault (core dumped)

Here's a test that any fix needs to be able to pass:

test.nalocf_last_column_all_NA <- function() {
  nacol <- NCOL(XDAT2)
  for (m in MODES) {
    xdat <- XDAT2
    xdat[,nacol] <- xdat[,nacol] * NA
    storage.mode(xdat) <- m
    zdat <- as.zoo(xdat)

    x <- na.locf(xdat)
    z <- na.locf(zdat)
    checkEquals(x, as.xts(z), check.attributes = TRUE)
  }
}