bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16589
Conversation
|
Tests run on Windows x64 but not on Linux. Still investigating. |
Modules/_ctypes/stgdict.c
Outdated
| Py_ssize_t length = dict->length; | ||
| StgDictObject *edict; | ||
| ffi_type * dummy_struct; |
There was a problem hiding this comment.
| ffi_type * dummy_struct; | |
| ffi_type *dummy_struct; |
Modules/_ctypes/stgdict.c
Outdated
| Py_ssize_t length = dict->length; | ||
| StgDictObject *edict; | ||
| ffi_type * dummy_struct; | ||
| ffi_type ** dummy_fields; |
There was a problem hiding this comment.
| ffi_type ** dummy_fields; | |
| ffi_type **dummy_fields; |
Modules/_ctypes/stgdict.c
Outdated
| /* Do separate allocations for now. */ | ||
| dummy_struct = PyMem_New(ffi_type, 1); | ||
| if (dummy_struct == NULL) { | ||
| PyErr_NoMemory(); |
There was a problem hiding this comment.
I think there should be a Py_DECREF(pair); here.
Modules/_ctypes/stgdict.c
Outdated
| } | ||
| dummy_fields = PyMem_New(ffi_type *, length + 1); | ||
| if (dummy_fields == NULL) { | ||
| PyErr_NoMemory(); |
There was a problem hiding this comment.
I think dummy_struct should be freed here. Also, pair should be decrefed.
There was a problem hiding this comment.
Well, the problem is that we're doing these allocations in a loop, and unless we keep some sort of linked list or dynamically allocated structure to keep track of all our allocations (both dummy_struct and dummy_fields in earlier iterations of the loop), we will leak this memory in an Out Of Memory (OOM) situation. I'm not aware that we currently do this type of recovery for these internal allocations in an OOM situation, but happy to be proved wrong.
There was a problem hiding this comment.
I will have a think about an alternative allocation strategy which allows an easier recovery, but it will be potentially a little harder to follow the logic compared to what's there now.
Agreed - please request a review from a core developer who you think might have the bandwidth to do this. I asked various people to review the earlier PR (for issue 22273) but didn't get many takers ... |
There was a problem hiding this comment.
LGTM. I've left some minor comments, but they aren't critical.
I did double-check the memory allocations and everything looks good. The only allocations were the 2 PySequence_GetItem() calls and the one PyMem_Malloc() call. They are cleaned up in all the right spots.
| if (pair == NULL) { | ||
| PyMem_Free(type_block); | ||
| return -1; | ||
| } | ||
| if (!PyArg_ParseTuple(pair, "UO|i", &name, &desc, &bitsize)) { |
There was a problem hiding this comment.
This check (but certainly not the call) might be redundant based on the earlier for loop.
There was a problem hiding this comment.
Although it shouldn't fail, I suppose one can't guarantee it? For that reason, I'd leave the check in, it makes the code a wee bit bigger but shouldn't really impact performance.
| if (!PyArg_ParseTuple(pair, "UO|i", &name, &desc, &bitsize)) { | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "'_fields_' must be a sequence of (name, C type) pairs"); | ||
| Py_XDECREF(pair); | ||
| PyMem_Free(type_block); | ||
| return -1; | ||
| } | ||
| dict = PyType_stgdict(desc); | ||
| if (dict == NULL) { |
There was a problem hiding this comment.
This check seems redundant based on the earlier for loop.
There was a problem hiding this comment.
As per earlier comment - would prefer to leave the check in for now.
|
Thanks for the feedback, @zooba , @ericsnowcurrently and @ZackerySpytz ! |
changed a couple of Py_XDECREF to Py_DECREF, and added some comments about possibly defensive checks.
|
I'll wait for the buildbots to pronounce on this merge before backporting to 3.8/3.7. |
|
Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
Sorry, @vsajip, I could not cleanly backport this to |
|
GH-16671 is a backport of this pull request to the 3.8 branch. |
|
Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7. |
|
Sorry @vsajip, I had trouble checking out the |
|
GH-16672 is a backport of this pull request to the 3.7 branch. |
|
I checked that at least two packages in Debian and Ubuntu, python-ipaddr and python-pysnmp4 now work again with this patch (only tested on the 3.7 branch). |
|
@doko42 Thanks for the feedback. |
…/unions. (pythonGH-16589) (pythonGH-16672) (cherry picked from commit e8bedbd)
…/unions.
https://bugs.python.org/issue38368