BUG: csv parsing with EOF byte on windows, Revert gh-16039 #16663

gfyoung · 2017-06-11T09:05:07Z

#16039 created a bug in which files containing byte-like data could break on Windows, as EOF characters mid-field (despite being quoted) would cause premature line breaks. Given that this PR was a performance patch, this commit can be safely reverted.

Closes #16559 (confirmed with unit test).

cc @dgwynne

jreback · 2017-06-11T15:51:30Z

pandas/_libs/src/parser/io.c

-    if (nbytes > fs->size) {
-        nbytes = fs->size;
-    }
+    *bytes_read = fread((void *)src->buffer, sizeof(char), nbytes, src->fp);


does this only read min(bytes left, nbytes)?

Yes, that is correct.

no, fread() will put up to sizeof(char) * nbytes into the buffer at src->buffer. if nbytes is greater than fs->size, then this will overflow. the rest of pandas may limit nbytes to fs->size before passing it to buffer_file_bytes(), but id argue that makes reading or auditing this code harder.

I'm pretty sure we'll hit end-of-file first, and it will terminate then, no?

if the file or the remainder of the file is shorter than fs->size, yes. that isn't the only situation buffer_file_bytes() is called in though.

say you have a 1MB file, and new_file_source() is called with 1000 as the buffer_size argument, then fs->size will be 1000 bytes long. if buffer_file_bytes() is called with nbytes as 2000, then you will read 2000 bytes into a 1000 byte long buffer because there at least that much data available in the file.

Okay, got it. I'll add that to my follow-up PR then.

jreback · 2017-06-11T15:53:51Z

looks reasonable. 3.6/windows is failing & pls run the csv asv's just to confirm no regression.

codecov · 2017-06-11T18:24:30Z

Codecov Report

Merging #16663 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #16663      +/-   ##
==========================================
+ Coverage   90.93%   90.96%   +0.02%     
==========================================
  Files         161      161              
  Lines       49269    49266       -3     
==========================================
+ Hits        44802    44813      +11     
+ Misses       4467     4453      -14

Flag	Coverage Δ
#multiple	`88.72% <ø> (+0.02%)`	⬆️
#single	`40.22% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/generic.py	`92.36% <0%> (+0.09%)`	⬆️
pandas/plotting/_converter.py	`65.05% <0%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a428d...045966a. Read the comment docs.

codecov · 2017-06-11T18:24:32Z

Codecov Report

Merging #16663 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #16663      +/-   ##
==========================================
- Coverage   90.93%   90.93%   -0.01%     
==========================================
  Files         161      161              
  Lines       49269    49270       +1     
==========================================
  Hits        44802    44802              
- Misses       4467     4468       +1

Flag	Coverage Δ
#multiple	`88.69% <100%> (-0.01%)`	⬇️
#single	`40.22% <0%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/parsers.py	`95.43% <100%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.23% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18a428d...69a4ad0. Read the comment docs.

gfyoung · 2017-06-11T18:48:13Z

3.6/windows is failing

Actually, it's the 2.7 build on Windows (it passes just fine on Python 3.6)

pls run the csv asv's just to confirm no regression.

Didn't see any noticeable regressions on my end.

pandas-devgh-16039 created a bug in which files containing byte-like data could break, as EOF characters mid-field (despite being quoted) would cause premature line breaks. Given that this PR was a performance patch, this commit can be safely reverted. Closes pandas-devgh-16559.

gfyoung · 2017-06-11T21:37:54Z

@jreback : All green and ready to go now.

jreback · 2017-06-11T23:03:47Z

pandas/_libs/src/parser/io.c

 /*
  On-disk FILE, uncompressed
 */

 void *new_file_source(char *fname, size_t buffer_size) {
    file_source *fs = (file_source *)malloc(sizeof(file_source));
-    if (fs == NULL) {


fs can still be a NULL pointer no?

Yes, it can be on some architectures. We should protect against that.

jreback · 2017-06-11T23:04:45Z

pandas/_libs/src/parser/io.c


    // Only allocate this heap memory if we are not memory-mapping the file
    fs->buffer = (char *)malloc((buffer_size + 1) * sizeof(char));

    if (fs->buffer == NULL) {
-        goto err_free;


don't you need to free fs here? (what about closing the fh)?

Yes, those would both be good to do here, if possible.

jreback · 2017-06-11T23:06:57Z

pandas/_libs/src/parser/io.c

        /* XXX Eventually remove this print statement. */
        fprintf(stderr, "new_file_buffer: mmap() failed.\n");
-        goto err_close;
+        free(mm);
+        mm = NULL;


shouldn't just return NULL here?

Yeah, you can just return NULL here IINM.

gfyoung · 2017-06-11T23:38:35Z

@jreback : I think you raise a couple of good points, but I think I would prefer to merge this (so that the bug is patched) and look into those afterwards as a follow-up, if that works?

jreback · 2017-06-11T23:42:34Z

I think you raise a couple of good points, but I think I would prefer to merge this (so that the bug is patched) and look into those afterwards as a follow-up, if that works?

that's fine. marking for 0.20.3 as this is a bug fix. if you run into issues and we have to make a bigger change can always just move to 0.21

yes bullet-proofing memory is an issue here (though hard to nail these types of things down).

Follow-up to pandas-devgh-16663.

dgwynne · 2017-06-12T01:18:09Z

hi guys, sorry im late to the party.

i am surprised by this, i thought that EOF was supposed to be represented outside the allowed set of characters you can have in a file, but after discussing this with a few people i get the impression that windows is special. as suggested by bug 5500, EOF on windows is 0x1a or 27, which can definitely end up in a file. on unix-like systems EOF tends to map to -1 which is supposed to outside what a char can represent.

one of the people i talked to pointed out that i dropped the binary flag when moving from fopen() ("b" in the mode string) to open(). in unix-like systems, the b flag is meaningless, but not on windows.

from what i can tell, ORing O_BINARY into the flags to open() might be enough to fix the problem. because O_BINARY may not be provided on other platforms, a common pattern for handling that is to put something like the following toward the top of the file:

#ifndef O_BINARY
#define O_BINARY 0
#endif /* O_BINARY */

gfyoung · 2017-06-12T01:21:25Z

@dgwynne :

windows is special

That's one way to put it. 😉

from what i can tell, ORing O_BINARY into the flags to open() might be enough to fix the problem

Hmm...interesting. Feel free to give it a shot. Then we can re-apply your PR with that patch if it works.

dgwynne · 2017-06-12T01:25:30Z

i don't have a windows box with a toolchain i can test with. if i generate a change against head, would you (or someone) be willing to try it?

gfyoung · 2017-06-12T01:41:16Z

We test the codebase against Appveyor, which runs Windows boxes. You can create an account for free, hook it up to your GitHub account, and it will run those tests for you before you submit the PR (or you could just submit immediately and let the tests run).

dgwynne · 2017-06-12T01:43:47Z

is there a test case that will catch this problem now?

gfyoung · 2017-06-12T01:46:27Z

Yes, I added one in this PR.

dgwynne · 2017-06-12T01:52:30Z

#16675 has been created. do i just wait now?

@jreback

* Revert "BUG: Revert gh-16039 (#16663)" This reverts commit c550372. * always treat files as binary to cope with windows and EOF. on windows, EOF can appear "in band" if the file is considered text. when moving from fread() to read(), i lost the "b" part of the mode. at the time i believed this was a nop, since unix doesnt treat files differently based on that flag. this adds O_BINARY to the flags to open to restore the behaviour lost when taking "b" away from fopen. if a platform doesn't provide O_BINARY, this defines it to 0 so it can still be used without effect later on in the code. * dont leak the fd in new_file_source() if buffer allocation fails. * reapply the test for EOF in the middle of a stream. part of c550372 * pass rb to _get_handle on python 3, otherwise stick to r. part of c550372 * replace goto with inline unwinding of state. requested by @jreback in #16675 feedback. * describe the fixes to the read_csv() backend and issue numbers. requested by @jreback in feedback on #16675

pandas-devgh-16039 created a bug in which files containing byte-like data could break, as EOF characters mid-field (despite being quoted) would cause premature line breaks. Given that this PR was a performance patch, this commit can be safely reverted. Closes pandas-devgh-16559. (cherry picked from commit c550372)

@jreback

…as-dev#16675) * Revert "BUG: Revert pandas-devgh-16039 (pandas-dev#16663)" This reverts commit c550372. * always treat files as binary to cope with windows and EOF. on windows, EOF can appear "in band" if the file is considered text. when moving from fread() to read(), i lost the "b" part of the mode. at the time i believed this was a nop, since unix doesnt treat files differently based on that flag. this adds O_BINARY to the flags to open to restore the behaviour lost when taking "b" away from fopen. if a platform doesn't provide O_BINARY, this defines it to 0 so it can still be used without effect later on in the code. * dont leak the fd in new_file_source() if buffer allocation fails. * reapply the test for EOF in the middle of a stream. part of c550372 * pass rb to _get_handle on python 3, otherwise stick to r. part of c550372 * replace goto with inline unwinding of state. requested by @jreback in pandas-dev#16675 feedback. * describe the fixes to the read_csv() backend and issue numbers. requested by @jreback in feedback on pandas-dev#16675 (cherry picked from commit d298414)

gh-16039 created a bug in which files containing byte-like data could break, as EOF characters mid-field (despite being quoted) would cause premature line breaks. Given that this PR was a performance patch, this commit can be safely reverted. Closes gh-16559. (cherry picked from commit c550372)

@jreback

* Revert "BUG: Revert gh-16039 (#16663)" This reverts commit c550372. * always treat files as binary to cope with windows and EOF. on windows, EOF can appear "in band" if the file is considered text. when moving from fread() to read(), i lost the "b" part of the mode. at the time i believed this was a nop, since unix doesnt treat files differently based on that flag. this adds O_BINARY to the flags to open to restore the behaviour lost when taking "b" away from fopen. if a platform doesn't provide O_BINARY, this defines it to 0 so it can still be used without effect later on in the code. * dont leak the fd in new_file_source() if buffer allocation fails. * reapply the test for EOF in the middle of a stream. part of c550372 * pass rb to _get_handle on python 3, otherwise stick to r. part of c550372 * replace goto with inline unwinding of state. requested by @jreback in #16675 feedback. * describe the fixes to the read_csv() backend and issue numbers. requested by @jreback in feedback on #16675 (cherry picked from commit d298414)

jreback added Bug IO CSV read_csv, to_csv Windows Windows OS labels Jun 11, 2017

jreback reviewed Jun 11, 2017

View reviewed changes

jreback changed the title ~~BUG: Revert gh-16039~~ BUG: csv parsing with EOF byte on windows, Revert gh-16039 Jun 11, 2017

gfyoung force-pushed the revert-16039 branch from 92035b1 to 045966a Compare June 11, 2017 18:24

gfyoung force-pushed the revert-16039 branch from 045966a to 1b14d38 Compare June 11, 2017 19:11

gfyoung force-pushed the revert-16039 branch from 1b14d38 to 69a4ad0 Compare June 11, 2017 19:29

jreback added this to the 0.20.3 milestone Jun 11, 2017

jreback reviewed Jun 11, 2017

View reviewed changes

jreback merged commit c550372 into pandas-dev:master Jun 11, 2017

jreback added the Needs Backport label Jun 11, 2017

gfyoung deleted the revert-16039 branch June 11, 2017 23:51

gfyoung added a commit to forking-repos/pandas that referenced this pull request Jun 12, 2017

MAINT: Buttress memory handling in C csv engine

fe0c53e

Follow-up to pandas-devgh-16663.

gfyoung added a commit to forking-repos/pandas that referenced this pull request Jun 12, 2017

MAINT: Buttress memory handling in C csv engine

86c5b9a

Follow-up to pandas-devgh-16663.

gfyoung mentioned this pull request Jun 12, 2017

MAINT: Buttress memory handling in C csv engine #16673

Closed

TomAugspurger removed the Needs Backport label Jul 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: csv parsing with EOF byte on windows, Revert gh-16039 #16663

BUG: csv parsing with EOF byte on windows, Revert gh-16039 #16663

gfyoung commented Jun 11, 2017 •

edited

Loading

jreback Jun 11, 2017

gfyoung Jun 11, 2017

dgwynne Jun 12, 2017

gfyoung Jun 12, 2017 •

edited

Loading

dgwynne Jun 12, 2017

gfyoung Jun 12, 2017

jreback commented Jun 11, 2017

codecov bot commented Jun 11, 2017

codecov bot commented Jun 11, 2017 •

edited

Loading

gfyoung commented Jun 11, 2017

gfyoung commented Jun 11, 2017

jreback Jun 11, 2017

gfyoung Jun 12, 2017

jreback Jun 11, 2017

gfyoung Jun 12, 2017

jreback Jun 11, 2017

gfyoung Jun 12, 2017 •

edited

Loading

gfyoung commented Jun 11, 2017

jreback commented Jun 11, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017 •

edited

Loading

dgwynne commented Jun 12, 2017

BUG: csv parsing with EOF byte on windows, Revert gh-16039 #16663

BUG: csv parsing with EOF byte on windows, Revert gh-16039 #16663

Conversation

gfyoung commented Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 11, 2017

codecov bot commented Jun 11, 2017

Codecov Report

codecov bot commented Jun 11, 2017 • edited Loading

Codecov Report

gfyoung commented Jun 11, 2017

gfyoung commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfyoung Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

gfyoung commented Jun 11, 2017

jreback commented Jun 11, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017

dgwynne commented Jun 12, 2017

gfyoung commented Jun 12, 2017 • edited Loading

dgwynne commented Jun 12, 2017

gfyoung commented Jun 11, 2017 •

edited

Loading

gfyoung Jun 12, 2017 •

edited

Loading

codecov bot commented Jun 11, 2017 •

edited

Loading

gfyoung Jun 12, 2017 •

edited

Loading

gfyoung commented Jun 12, 2017 •

edited

Loading