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

Prevent writing out of bounds when final column is entirely NA #235

Merged
merged 2 commits into from Apr 19, 2018

Conversation

Projects
None yet
2 participants
@TomAndrews
Contributor

TomAndrews commented Apr 18, 2018

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! 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.

Owner

joshuaulrich commented Apr 18, 2018

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.

@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich Apr 18, 2018

Owner

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 fromLast = TRUE.

It would be great if you add these tests to your PR and fix whatever else they uncover. Then please use git commit --amend to update your PR commit (instead of adding new commits). You'll also need to add --force to your push command.

Owner

joshuaulrich commented Apr 18, 2018

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 fromLast = TRUE.

It would be great if you add these tests to your PR and fix whatever else they uncover. Then please use git commit --amend to update your PR commit (instead of adding new commits). You'll also need to add --force to your push command.

@TomAndrews

This comment has been minimized.

Show comment
Hide comment
@TomAndrews

TomAndrews Apr 18, 2018

Contributor

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.

Contributor

TomAndrews commented Apr 18, 2018

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.

@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich Apr 18, 2018

Owner

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 if (_first == nr + j*nr) _first--;.

Owner

joshuaulrich commented Apr 18, 2018

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 if (_first == nr + j*nr) _first--;.

@joshuaulrich

This comment has been minimized.

Show comment
Hide comment
@joshuaulrich

joshuaulrich Apr 18, 2018

Owner

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".

Owner

joshuaulrich commented Apr 18, 2018

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".

TomAndrews added some commits 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
Index multicolumn STRSXP correctly in firstNonNACol
This was missing the column index and so was only checking the first
column no matter the requested column

@joshuaulrich joshuaulrich merged commit e30854a into joshuaulrich:master Apr 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@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