BUG: Use size_t to avoid array index overflow; add missing malloc of error_msg
State: closed

Fix a few locations where a parser's `error_msg` buffer is written to
without having been previously allocated. This manifested as a double
free during exception handling code making use of the `error_msg`.

Additionally, use `size_t/ssize_t` where array indices or lengths will
be stored. Previously, int32_t was used and would overflow on columns
with very large amounts of data (i.e. greater than INTMAX bytes).

 - [ ] closes #16798 
 - [ ] tests added / passed
 - [ ] passes ``git diff upstream/master -u -- "*.py" | flake8 --diff``
 - [ ] whatsnew entry


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

Comments:
Very honestly, I'm not sure I'm comfortable with this many unsigned integers. My rule of thumb is:

* Use int for things that are unlikely to ever be big enough to worry about an int32 overflow (like the number of columns in a table -- if you had 2 billion columns, you would probably run into other problems sooner)
* Use `int64_t` for things that are expected to be able to overflow a 32-bit signed integer
* Use `size_t` exclusively when dealing with malloc, memcpy, and other C standard library functions, and for handling C++ loop indices in the STL (`for (size_t i = 0; i < foo.size(); ++i)`)

In Google's C++ style guide they say

```
Some people, including some textbook authors, recommend using unsigned types to represent
numbers that are never negative. This is intended as a form of self-documentation. However, in C, 
the advantages of such documentation are outweighed by the real bugs it can introduce. Consider:

for (unsigned int i = foo.Length()-1; i >= 0; --i) ...

This code will never terminate! Sometimes gcc will notice this bug and warn you, but often it will not.
Equally bad bugs can occur when comparing signed and unsigned variables. Basically, C's type
promotion scheme causes unsigned types to behave differently than one might expect.

So, document that a variable is non-negative using assertions. Don't use an unsigned type.
```
> Very honestly, I'm not sure I'm comfortable with this many unsigned integers.

That's fair.  In that case, `ssize_t` would be preferable to `size_t`.
`ssize_t` isn't a standard integer type; it's a POSIX typedef. For best cross-platform compatibility the best integer to use for values that are expected to be large is `int64_t`
@wesm FWIW, almost all of the changed fields are `malloc`-ed, `memcpy`-ed or `realloc`ed (the source of the original bug). I tried to keep the surface area of the change as small as possible but still address the places where a) an int32_t would overflow and b) one would reasonably expect and `(s)size_t`.

For me, `size_t` was always seemed natural because a) it's what `malloc()` expects and b) it's what `sizeof()` returns. The reason why it is (and must be) unsigned, then, becomes clear: if it weren't, one wouldn't be able to allocate the full addressable space via `malloc()` or accurately report the size of a type greater than `int64_t` max, while both are perfectly valid uses of the respective functions (IMHO).

That said, it sounds like you're -1 on size_t, which is fine. I'll fix all changed types to be `int64_t`.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=h1) Report
> Merging [#17040](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/8e582254e3bbdd717ec8193364420913a7fc786d?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17040/graphs/tree.svg?token=eZ4WkYLtcO&width=650&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17040      +/-   ##
==========================================
- Coverage   91.02%      91%   -0.02%     
==========================================
  Files         161      161              
  Lines       49308    49308              
==========================================
- Hits        44883    44874       -9     
- Misses       4425     4434       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.78% <ø> (ø)` | :arrow_up: |
| #single | `40.19% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=footer). Last update [8e58225...e04d12a](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=h1) Report
> Merging [#17040](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/8e582254e3bbdd717ec8193364420913a7fc786d?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/17040/graphs/tree.svg?token=eZ4WkYLtcO&src=pr&height=150&width=650)](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #17040      +/-   ##
==========================================
+ Coverage   91.02%   91.03%   +<.01%     
==========================================
  Files         161      161              
  Lines       49308    49351      +43     
==========================================
+ Hits        44883    44927      +44     
+ Misses       4425     4424       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.81% <100%> (+0.03%)` | :arrow_up: |
| #single | `40.26% <66.66%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/conftest.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvbmZ0ZXN0LnB5) | `96.77% <100%> (+0.34%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/missing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbWlzc2luZy5weQ==) | `84.36% <0%> (-0.89%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92% <0%> (-0.31%)` | :arrow_down: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `91.86% <0%> (-0.17%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.11%)` | :arrow_down: |
| [pandas/core/sparse/array.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2FycmF5LnB5) | `91.42% <0%> (-0.05%)` | :arrow_down: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `96.92% <0%> (-0.03%)` | :arrow_down: |
| [pandas/core/sparse/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL2ZyYW1lLnB5) | `94.24% <0%> (ø)` | :arrow_up: |
| [pandas/io/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree#diff-cGFuZGFzL2lvL2h0bWwucHk=) | `84.85% <0%> (ø)` | :arrow_up: |
| ... and [10 more](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=footer). Last update [8e58225...6a1ba23](https://codecov.io/gh/pandas-dev/pandas/pull/17040?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

As an aside, Google's style guide seems a bit all-over-the-map. They suggest not using unsigned types and in the very same breath dictate that assertions should be preferred for "documenting" a value must be non-negative. Of course, the only problem with that is `assert` is a macro dependent on the `NDEBUG` macro which itself is not part of C++ standard library. Also, I can't imagine finding a widely used version of `gcc` (well, `g++` I guess) that wouldn't warn about the type difference in a comparison of the form they present. If the compiler can't make *that* determination, you may as well just give up... :trollface: 

Enough bike-shedding, though! I made the requested changes and pushed. Please review at your convenience.
@jeffknupp : Do you have a test to reproduce the issue (I know that was problematic was the issue, but I'm curious what the current status on that is now)?

Also, add an entry in the `whatsnew` .
@jeffknupp : Can you check that the issue in #14696 is handled by your changes?
Yes, it is handled by this change.

Thanks,
Jeff

On Jul 20, 2017, 7:17 PM -0400, gfyoung <notifications@github.com>, wrote:
> @jeffknupp : Can you check that the issue in #14696 is handled by your changes?
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

@jeffknupp sorry about _my_ bike shedding. I agree it's often a lot of worrying over nothing but thanks for indulging my OCDs
@gfyoung Added what I think is an appropriate set of entries in `whatsnew` as well as some heavy-duty horizontal alignment 😉.

I have a very simple test (which I gave to @wesm and believe was linked to from the original issue) which would cause a simple script of the form:

```
import pandas as pd

try:
    df = pd.read_csv('test_file.gz2', low_memory=False)
except:
    print('Failed')
```

to core (and, after the allocation issue was fixed, to simply print an "out of memory" error). It required a data file that was ~300 MB after bzip2, for which there should be a link.
> as well as some heavy-duty horizontal alignment

It's beautiful, thanks. :smile: 

> I have a very simple test

Ah, okay.  It seems like we would have to add this to the repository, though I don't want to commit something like that given the file size without further discussion.  Thoughts?
@wesm was faced with a similar issue for a similar problem in apache/arrow, which is where this first manifest itself. Let me dig around and see what he did (though I remember an email in the past few days specifically mentioning that the test included would only be run if a specific flag was set, and wouldn't run by default).
So, it looks like it works aside from Python 2.7 on Windows? I'm looking at the failing tests now in appveyor. Would like to get this buttoned up tonight but may have to punt to the morning.
@jreback Right, that's the testing solution I originally proposed on the issue. Marking it as a "slow" test, though, won't work, as someone looking to be exhaustive in their testing who includes the slow tests will be surprised when one fails because "slow" actually meant "bit" in this case. 

Is there already a separate category for "large memory" tests? It looks like that's what @wesm did for a very similar issue involving the same test data in apache/arrow
Also, I have a reasonable idea of the fixes required to get the appveyor tests to pass but is there any way of testing these fixes without direct access to a windows environment? I don't want to get into a multi-hour push change, wait for appveyor to test PR, apply small fix, push change dev cycle here...
> Also, I have a reasonable idea of the fixes required to get the appveyor tests to pass but is there any way of testing these fixes without direct access to a windows environment?

Spinning up a Windows VM might be your way to go.  Then you can run the tests directly in a Windows environment on your computer.

However, I think running tests on `Appveyor` is the only way to go ATM if you don't want to go through all that work.  What you could do (to avoid having to wait for other builds) is to make an account on AppVeyor and make a new branch that mirrors this one in the PR.  Then you you can make whatever changes you want on that branch to get tests to pass (without competing with other PR's) and then merge them into the branch for this PR.
As I write this, I notice that AppVeyor is passing for you now, but Travis is broken, so I guess the issue is kind of moot for now. :smile: 
@wesm @jreback is it possible to merge this and leave the original issue open until I submit a suitable patch to demonstrate a negative test for this change? Basically, I need to get this merged into pandas ASAP so that a) I can do a dev install from the official pandas repo in my project and b) want to stop doing a) ASAP, and I don't want the actual fix to fall by the wayside as we determine the best way to test.
(Or merge and leave the PR open with the understanding I will work on the test this weekend or early next week)
@jeffknupp : The next release (`0.21`) won't come out until end of September AFAIK, so I don't think there is any rush to merge this.  Feel free to take your time on this :smile: 
I've tagged it for `0.21` so we won't forget it :smile: 
you can simply install from your fork the same way you would from master just a different branch 
note that we DO test on linux-32, but on nightly (and no way to actually test this a-priori, so would only show up after merging).

e.g. see https://travis-ci.org/MacPython/pandas-wheels
@jreback Note that the issue is not related to the underlying architecture of the machine. Rather, it's due to the fact that using `int`s in calls to `malloc`/`realloc` exposes you to integer overflow issues when trying to allocate anything more than `INT_MAX / 2` bytes (a 32-bit system can, by definition, store an address greater than 2^16).

To trigger the issue, one simply needs to create a CSV where at least one column is larger than `INT_MAX` bytes. However, since that value is typically exactly 2 GB, you'll need a machine with at least that much RAM. In reality, you'll need quite a bit more since a) you need to make at least one copy of the data for the parser and b) you need to make sure you don't start swapping so bad you take down the system while also avoid being killed by the `oom killer`.

So, tl;dr, any test for this issue will need to be run on a machine with sufficient RAM, where "sufficient" is likely in the > 6GB range.
Hello @jeffknupp! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on July 23, 2017 at 14:07 Hours UTC
@jreback @gfyoung OK, this should be considered complete.

I added a new pytest option, `--run-highmemory` which _enables_ tests marked `high_memory`. They are not run by default as I don't want to destroy your numerous CI environments 😄 .

The new test, marked as `high_memory`, is crude but effective. The whole test takes roughly 8GB on my MBP and maybe 20 seconds to complete. I have confirmed:

- If run against current `master`, the `pytest` crashes (which I think we can consider "a failure") as was observed in the issue
- If run on the new code with `--run-highmemory` enabled, the test passes

Note that since I'm not aware of a nontrivial way of constructing a test to see if the interpreter itself will crash, test coverage will likely complain that the two new `error_msg` allocations are not covered (since this code fixes both the crashes at those locations and the code that was incorrectly invoking them in the first place). I hope that's OK as I'm not sure how much more time I can devote to this specific issue.
> I hope that's OK as I'm not sure how much more time I can devote to this specific issue.

Understood.  Could you at least patch the lint errors on Travis right now?  It would be great to get all builds passing. :smile: 
@gfyoung because you asked so nicely... 😄 . A bit of acrobatics for 80-character width. `120` is the new `80`.