pixz doesn't check for out-of-memory

[bremner]

cppcheck complains as follows

[common.c:215]: (error) Common realloc mistake: 'gFileIndexBuf' nulled but not freed upon failure

I didn't follow the code outside that function, but just looking at that line it does look like that could be a problem if realloc returns NULL.

[vasi]

Thanks for the bug report. You're correct in the strict sense, if the realloc fails, pixz will crash. Unfortunately, since pixz doesn't do any checking for out-of-memory conditions, ANY malloc/realloc failure will cause a crash.

I guess this could be 'fixed' by just going through the code and calling die() if any malloc fails, but that doesn't really help. Ideally we'd preallocate all memory, but that would be a huge and invasive change. Even more-ideally, we'd also try to prevent the system from swapping.

[wookietreiber]

I think, it is good practice to use a tool like the abovementioned cppcheck. I am working on adding this to travis.

[wookietreiber]

@vasi I have prepared a pull request that both adds a cppcheck test to the source directory (good to have this anyway) and fixes this issue by implementing a check on the result of the realloc function call.

If you are OK with this, please let me know or merge the pull request #42 yourself. I am asking you specifically because you have shown hesitance on the issue in a previous comment.

IMHO, in the end, it is always better to give the end-user a reasonable error message than letting pixz error out by itself while dereferencing NULL.

[wookietreiber]

@vasi ping

[vasi]

Sorry, I didn't see this earlier. I think it's useful, you're certainly right that it's better to provide an error than just crash. Feel free to commit!

I do want to still push for a real solution, though:

1. We can try to predict when memory will become a problem. The amount of memory pixz uses is roughly proportional to queue-size * buffer-size. If that number exceeds available physical memory, we should print a warning, and maybe scale down one of these factors.
2. We can also attempt to do real recovery from OOM. For the index, we could flush what we have to a temp flle.

[wookietreiber]

I have added a TODO comment so we can address this later. We should probably open a new issue to address OOM prevention and recovery as a future feature. I will do this.

[cicku]

SO pixz still has OOM issue?

[wookietreiber]

OOM is usually considered to be a fatal error, because, in most cases, there is nothing you can do about it but crash. Often, applications do not even handle this error and let it fall through to the application caller, e.g. the terminal / shell process.

Being able to prevent OOM or even recover from it highly depends on the application. Whether or not we can do this at all in a sane way must be researched. We will keep you up-to-date on this progress in #44.

What I did here to solve this issue, is just checking the realloc call and fail-fast with an appropriate error. This is a common C / C++ practice that you should apply as was reported initially by @bremner with the cppcheck tool.