bpo-40020: fix realloc leak on failure in growable_comment_array_add#19083
Conversation
Realloc returns a null pointer on failure, and then growable_comment_array_deallocate crashes later when it dereferences it.
|
are the workflow tests the same thing as the patchcheck tests? |
corona10
left a comment
corona10
left a comment
There was a problem hiding this comment.
I am LGTM with this change
The realloc(3) says that
If realloc() fails the original block is left untouched; it is not freed or moved
Please ignore the test failure, it's the known issue.
|
@vstinner Please take a look :) |
| arr->size *= 2; | ||
| arr->items = realloc(arr->items, arr->size * sizeof(*arr->items)); | ||
| if (!arr->items) { | ||
| void *const new_items_array = realloc(arr->items, arr->size * sizeof(*arr->items)); |
There was a problem hiding this comment.
I don't see the purpose of "const" here. I suggest to remove it. realloc() return type is void *.
There was a problem hiding this comment.
I mean, hey, if you guys don't like const correctness, I can do that. Thankfully, I have enough control over my OCD that it works for me :)
There was a problem hiding this comment.
Actually, while I'm here, should I also not double the array size on the failure path?
And should I squash the multiple commits?
There was a problem hiding this comment.
"hey, if you guys": please use a more inclusive expressions, see https://heyguys.cc/ ;-)
"don't like const correctness": it's assigned to arr->items 3 lines below but growable_comment_array.items isn't constant. I don't see the value of using const here.
You should read https://theartofmachinery.com/2019/08/12/c_const_isnt_for_performance.html "Why const Doesn't Make C Code Faster" :-)
There was a problem hiding this comment.
should I also not double the array size on the failure path?
Right, arr->size should only be modified on success. Replace maybe realloc(arr->items, arr->size * sizeof(*arr->items)) with realloc(arr->items, (arr->size * 2) * sizeof(*arr->items)).
And should I squash the multiple commits?
That's harmful for reviewers. I prefer to see new commits.
We always squash all commits of a PR when it's merged.
There was a problem hiding this comment.
Let me explain differently: your usage of const is correct. But IMHO it's not worth it, and I see it more as an annoyance for readers.
|
Once this change will be merged, I would be interested to see a second change to replace realloc() and free() with PyMem_RawRealloc() and PyMem_RawFree(), to benefit of Python builtin memory debugger. Like detecting buffer underflow and overflow in debug mode: https://docs.python.org/dev/c-api/memory.html#c.PyMem_SetupDebugHooks See PYTHONMALLOC=debug. tracemalloc may also benefit of that, but mostly to measure the peak memory usage. I expect that the parser memory is released before the user gets back control on the program. |
As an interesting side note, I think code analysis only found this because the code used raw realloc and free, since |
|
Ok, how does this look? |
|
I merged your PR, thanks. I don't think that it's worth it to backport the fix. |
|
Backport? Nah, no need. |
5 participants
Fixes bpo-40020. I have signed the CLA, but I don't know if it's active yet.
I don't have make installed on this machine, but I'm setting it up right now to get the patchcheck run. Will update.
realloc returns a null pointer on failure, and then growable_comment_array_deallocate crashes later when it dereferences it.
https://bugs.python.org/issue40020