BUG: set src->buffer = NULL after garbage collecting it in buffer_rd_…
State: closed

Issue #12098

Add src->buffer = NULL; after garbage collecting src->buffer in the buffer_rd_bytes routine in io.c to fix the segfault


Labels:
name: Bug
name: IO CSV
description: read_csv, to_csv

Comments:
How large is the data file that's required to reproduce this? Since we understand the underlying cause now, we may be able to construct a much smaller one, maybe by appending garbage bytes to the end of a valid gzip file (which will cause the decompressor to fail and raise an exception, triggering this error, hopefully). 

Which is to say I would hate to merge this without a unit test. 

the example is 10k bytes 
so could use that or try to construct one

I tried removing parts of the data string from the original poster and tried creating a small corrupted gzip file but was unable to reproduce the segfault.  I'll try a few more small corrupted gzip files to see if I can get something small enough to fit in a unit test.

This is tricky because the _first_ file read has to succeed to hit this bug, and the second to fail. So it's a buffer sizing issue (gzip has an internal buffer size)

Triggering the segfault varies depending on which python and pandas versions are used.  When I run the example with python2.7.11 and pandas 0.17.1,  read_csv executes 5 times in the loop before the segfault occurs.  The loop executes 9 times before the segfault when run with the latest master of pandas.  On a different Mac running python 2.7.10 and pandas 0.17.1 it loops once then segfaults on the second call to read_csv.  The example runs without segfaulting under python 3.5.1 using BytesIO instead of StringIO.

That's fine, as long as the test is consistently flaky on some platforms. Since we are dealing with a double free issue here it will cause non-deterministic failure. If you run valgrind you'll probably see the invalid deallocation

I was able to make a test with a shorter data string that segfaults without the src->buffer = NULL; bug fix

```
def test_buffer_rd_bytes(self):
    # GH 12098
    # src->buffer can be freed twice leading to a segfault if a corrupt 
    # gzip file is read with read_csv and the buffer is filled more than
    # once before gzip throws an exception

    data = '\x1F\x8B\x08\x00\x00\x00\x00\x00\x00\x03\xED\xC3\x41\x09' \
           '\x00\x00\x08\x00\xB1\xB7\xB6\xBA\xFE\xA5\xCC\x21\x6C\xB0' \
           '\xA6\x4D' + '\x55' * 267 + \
           '\x7D\xF7\x00\x91\xE0\x47\x97\x14\x38\x04\x00' \
           '\x1f\x8b\x08\x00VT\x97V\x00\x03\xed]\xefO'
    for i in range(100):
        try:
            _ = pd.read_csv(StringIO(data),
                            compression='gzip',
                            delim_whitespace=True)
        except Exception as e:
            pass
```

I put the test in TestCParserHighMemory and in TestCParserLowMemory.  If that is the proper place for them I will update my PR

looks good, though use `self.read_csv` which will trigger calls based on the parser engine (e.g. c/python). 

@selasley thanks!

@selasley Looking at this test now, the `except Exception: pass` is hiding what looks like a bytes/str problem in py3.  Was this test/problem supposed to be py2-specific?
I believe it was meant for python2 based on the Jan 25 comment.
@jreback can you confirm the "this test is only relevant in py2" hypothesis?  or suggest someone else to ask about it?
yeah might be py2 only
@TomAugspurger or @jorisvandenbossche can I get a third opinion before ripping this test out?
No idea. I don't think I'm familiar with the original issue.