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.

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

Commit 02300bdc9375c868bc31debe6f5d748cd03aa391 introduces a segfault #234

Closed
TomAndrews opened this Issue Apr 18, 2018 · 1 comment

Comments

Projects
None yet
2 participants
@TomAndrews
Contributor

TomAndrews commented Apr 18, 2018

Description

Commit 02300bd 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!

TomAndrews added a commit to TomAndrews/xts that referenced this issue Apr 18, 2018

Prevent writing out of bounds when final column in entirely NA
If a column is entirely NA, then firstNonNACol will return the index
of the first entry of the next column.  In the case where the final
column is entirely NA then this will result in writing off the end of
the allocated result.

Fix by subtracting one from the index in this case.

Hopefully fixes #234
@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich Apr 18, 2018

Owner

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)
  }
}
Owner

joshuaulrich commented Apr 18, 2018

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)
  }
}

TomAndrews added a commit to TomAndrews/xts that referenced this issue Apr 18, 2018

Prevent writing out of bounds when final column in entirely NA
If a column is entirely NA, then firstNonNACol will return the index
of the first entry of the next column.  In the case where the final
column is entirely NA then this will result in writing off the end of
the allocated result.

Fix by subtracting one from the index in this case.

Hopefully fixes #234

@joshuaulrich joshuaulrich added the bug label Apr 18, 2018

TomAndrews added a commit to TomAndrews/xts that referenced this issue Apr 19, 2018

Prevent writing out of bounds when final column in entirely NA
If a column is entirely NA, then firstNonNACol will return the index
of the first entry of the next column.  In the case where the final
column is entirely NA then this will result in writing off the end of
the allocated result.

Fix by subtracting one from the index in this case.

Hopefully fixes #234

@joshuaulrich joshuaulrich closed this in #235 Apr 19, 2018

@joshuaulrich joshuaulrich added this to the Release 0.11-0 milestone Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment