Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] CSV Reader: Fix a segfault when byte range aligns with a page #1044

Merged
merged 6 commits into from
Feb 27, 2019
Merged

[REVIEW] CSV Reader: Fix a segfault when byte range aligns with a page #1044

merged 6 commits into from
Feb 27, 2019

Conversation

vuule
Copy link
Contributor

@vuule vuule commented Feb 25, 2019 •
edited
Loading

Fixes rapidsai/dask-cudf#102

  • Remove the extra byte that was copied from the last chunk in launch_storeRecordStart(). The intent was to copy an extra byte for all chunks except the last one, but the implementation did the opposite. This cause a segfault when the map size aligns with a page.
  • Remove the code to explicitly handle \r\n terminator in storeRecordStart() and countRecords(), since regular check (\n) can cover this case too.
    *When mapping the file, num_bytes is now updated to account for page padding in all cases.
    *Add a test for the failing case.

@vuule
Remove the extra byte added to chunks to acount for \r\n. The byte is…
f77445b 
… not needed to correctly parse with this terminator. Fixes a segfault when map size alignes with a page.
@vuule vuule mentioned this pull request Feb 25, 2019
Closed
@kkraus14 kkraus14 added 2 - In Progress Currently a work in progress cuIO cuIO issue labels Feb 26, 2019
@vuule vuule mentioned this pull request Feb 26, 2019
Closed
@vuule vuule changed the title [WIP] CSV Reader: Fix a segfault when byte range alings with a page [REVIEW] CSV Reader: Fix a segfault when byte range alings with a page Feb 26, 2019
j-ieong
j-ieong suggested changes Feb 26, 2019
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
j-ieong
j-ieong approved these changes Feb 27, 2019
View reviewed changes
Copy link
Contributor

@j-ieong j-ieong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to fix the typo in the PR title as well.

@vuule vuule changed the title [REVIEW] CSV Reader: Fix a segfault when byte range alings with a page [REVIEW] CSV Reader: Fix a segfault when byte range aligns with a page Feb 27, 2019
@vuule
Copy link
Contributor Author

vuule commented Feb 27, 2019

You need to fix the typo in the PR title as well.

Fixed.

@mjsamoht mjsamoht merged commit 94fa0bc into rapidsai:branch-0.6 Feb 27, 2019
@vuule vuule deleted the bug-ext-csv-page-aligned-byte-range branch February 27, 2019 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-ieong j-ieong j-ieong approved these changes

@mjsamoht mjsamoht mjsamoht approved these changes

Assignees
No one assigned
Labels
2 - In Progress Currently a work in progress cuIO cuIO issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vuule @mjsamoht @j-ieong @kkraus14