Prevent writing out of bounds when final column is entirely NA #235
Conversation
|
Thanks for the report! I'll investigate. In the future, please wait for feedback before opening a PR. The rationale for this is in the contributing guide. The change is small in this case, but changes in C code are less likely to be "trivial" than changes in R code. |
|
Also, the unit test I added to your issue fails for your branch on "character" vectors. I suspect you simply missed that branch in the code. There should also be tests for calls where It would be great if you add these tests to your PR and fix whatever else they uncover. Then please use |
TomAndrews
force-pushed
the
234-fix-na-locf-segfault
branch
from
95e49af to
f034f2b
Compare
|
It turned out the problem was in firstNonNACol - I don't think the indexing was correct in the STRSXP case. I've put my change in a different commit so it's easy for you to see but can squash and push if you prefer. |
|
Yeah, it looks like you are correct. And I agree that it should be in a different commit in this case, since it's a different problem. I would only have squashed if the new commit only added another |
|
I've merged your other PR, so go ahead and rebase this one when you have a chance. I can do it locally, but when I've done that in the past GitHub has marked the PR as "closed" instead of "merged"... and I'd prefer you get a "thumbs up". |
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 joshuaulrich#234
This was missing the column index and so was only checking the first column no matter the requested column
TomAndrews
force-pushed
the
234-fix-na-locf-segfault
branch
from
c327000 to
e30854a
Compare
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