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

[selasley]

Issue #12098

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

[wesm]

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

[wesm]

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

[jreback]

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

[selasley]

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.

[wesm]

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)

[jreback]

more like, segfault in repeated reading of gziped input file.

[selasley]

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.

[wesm]

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

[selasley]

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

[jreback]

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

[jreback]

@selasley thanks!

[jbrockmendel]

@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?

[selasley]

I believe it was meant for python2 based on the Jan 25 comment.

[jbrockmendel]

@jreback can you confirm the "this test is only relevant in py2" hypothesis? or suggest someone else to ask about it?

[jreback]

yeah might be py2 only

[jbrockmendel]

@TomAugspurger or @jorisvandenbossche can I get a third opinion before ripping this test out?

[TomAugspurger]

No idea. I don't think I'm familiar with the original issue.