bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16589

vsajip · 2019-10-04T23:43:21Z

…/unions.

https://bugs.python.org/issue38368

…/unions.

vsajip · 2019-10-05T00:11:14Z

Tests run on Windows x64 but not on Linux. Still investigating.

ZackerySpytz · 2019-10-05T15:01:05Z

Modules/_ctypes/stgdict.c

                Py_ssize_t length = dict->length;
                StgDictObject *edict;
+                ffi_type * dummy_struct;


Suggested change

ffi_type * dummy_struct;

ffi_type *dummy_struct;

ZackerySpytz · 2019-10-05T15:01:13Z

Modules/_ctypes/stgdict.c

                Py_ssize_t length = dict->length;
                StgDictObject *edict;
+                ffi_type * dummy_struct;
+                ffi_type ** dummy_fields;


Suggested change

ffi_type ** dummy_fields;

ffi_type **dummy_fields;

ZackerySpytz · 2019-10-05T15:01:54Z

Modules/_ctypes/stgdict.c

+                /* Do separate allocations for now. */
+                dummy_struct = PyMem_New(ffi_type, 1);
+                if (dummy_struct == NULL) {
+                    PyErr_NoMemory();


I think there should be a Py_DECREF(pair); here.

ZackerySpytz · 2019-10-05T15:03:13Z

Modules/_ctypes/stgdict.c

+                }
+                dummy_fields = PyMem_New(ffi_type *, length + 1);
+                if (dummy_fields == NULL) {
+                    PyErr_NoMemory();


I think dummy_struct should be freed here. Also, pair should be decrefed.

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.

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.

…ure.

zooba

I'd like to have one more person go over this line-by-line as well to track the memory usage, but I didn't spot anything that seemed wrong.

vsajip · 2019-10-07T21:21:57Z

I'd like to have one more person go over this line-by-line as well to track the memory usage

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

ericsnowcurrently

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.

Modules/_ctypes/stgdict.c

ericsnowcurrently · 2019-10-08T16:21:12Z

Modules/_ctypes/stgdict.c

+            if (pair == NULL) {
+                PyMem_Free(type_block);
+                return -1;
+            }
            if (!PyArg_ParseTuple(pair, "UO|i", &name, &desc, &bitsize)) {


This check (but certainly not the call) might be redundant based on the earlier for loop.

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.

ericsnowcurrently · 2019-10-08T16:21:29Z

Modules/_ctypes/stgdict.c

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


This check seems redundant based on the earlier for loop.

As per earlier comment - would prefer to leave the check in for now.

vsajip · 2019-10-08T20:08:34Z

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.

vsajip · 2019-10-08T20:59:51Z

I'll wait for the buildbots to pronounce on this merge before backporting to 3.8/3.7.

miss-islington · 2019-10-09T04:58:04Z

Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2019-10-09T04:58:11Z

Sorry, @vsajip, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker e8bedbddadaa86be6bd86dc32dbdbd53933a4988 3.8

bedevere-bot · 2019-10-09T05:17:21Z

GH-16671 is a backport of this pull request to the 3.8 branch.

miss-islington · 2019-10-09T05:20:47Z

Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2019-10-09T05:20:48Z

Sorry @vsajip, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker e8bedbddadaa86be6bd86dc32dbdbd53933a4988 3.7

bedevere-bot · 2019-10-09T05:31:01Z

GH-16672 is a backport of this pull request to the 3.7 branch.

…/unions. (GH-16589) (GH-16671) (cherry picked from commit e8bedbd)

…/unions. (GH-16589) (GH-16672) (cherry picked from commit e8bedbd)

doko42 · 2019-10-09T08:59:29Z

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

vsajip · 2019-10-09T10:55:41Z

@doko42 Thanks for the feedback.

…/unions. (pythonGH-16589) (pythonGH-16672) (cherry picked from commit e8bedbd)

pythonGH-16589)

bpo-38368: Added fix for ctypes crash when handling arrays in structs…

03934da

…/unions.

vsajip added type-bug An unexpected behavior, bug, or error skip news labels Oct 4, 2019

vsajip requested a review from zooba October 4, 2019 23:43

the-knights-who-say-ni added the CLA signed label Oct 4, 2019

bedevere-bot added the awaiting core review label Oct 4, 2019

vsajip removed the request for review from zooba October 5, 2019 00:04

Simplified handling of ctypes structs/unions containing arrays.

5c947c7

vsajip requested a review from zooba October 5, 2019 11:40

ZackerySpytz reviewed Oct 5, 2019

View reviewed changes

vsajip added 5 commits October 5, 2019 16:24

Added some more test logic.

fd976d3

Changes in response to review comments.

2b07ffe

Reverted to single-allocation strategy for easier backing out on fail…

4942f5c

…ure.

Amended incorrect assertion.

b440691

Moved assertion out of condition.

8bc7ca9

zooba approved these changes Oct 7, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 7, 2019

ericsnowcurrently approved these changes Oct 8, 2019

View reviewed changes

Changed the name of a #defined constant to be more descriptive,

f2f02a4

changed a couple of Py_XDECREF to Py_DECREF, and added some comments about possibly defensive checks.

vsajip merged commit e8bedbd into python:master Oct 8, 2019

vsajip deleted the fix-38368 branch October 8, 2019 20:59

bedevere-bot removed the awaiting merge label Oct 8, 2019

vsajip added the needs backport to 3.8 label Oct 9, 2019

miss-islington assigned vsajip Oct 9, 2019

vsajip mentioned this pull request Oct 9, 2019

[3.8] bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16671

Merged

bedevere-bot removed the needs backport to 3.8 label Oct 9, 2019

vsajip added the needs backport to 3.7 label Oct 9, 2019

bedevere-bot removed the needs backport to 3.7 label Oct 9, 2019

vsajip added a commit that referenced this pull request Oct 9, 2019

bpo-38368: Added fix for ctypes crash when handling arrays in structs…

d004a5b

…/unions. (GH-16589) (GH-16671) (cherry picked from commit e8bedbd)

vsajip added a commit that referenced this pull request Oct 9, 2019

bpo-38368: Added fix for ctypes crash when handling arrays in structs…

129c2b3

…/unions. (GH-16589) (GH-16672) (cherry picked from commit e8bedbd)

ned-deily pushed a commit to ned-deily/cpython that referenced this pull request Oct 14, 2019

bpo-38368: Added fix for ctypes crash when handling arrays in structs…

1c61f2c

…/unions. (pythonGH-16589) (pythonGH-16672) (cherry picked from commit e8bedbd)

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38368: Added fix for ctypes crash when handling arrays in structs… (

2cf8177

pythonGH-16589)

eric-wieser mentioned this pull request Jul 7, 2022

ctypes: memoryview gives incorrect PEP3118 format strings for both packed and unpacked structs #76961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16589

bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16589

vsajip commented Oct 4, 2019 •

edited by bedevere-bot

Loading

vsajip commented Oct 5, 2019

ZackerySpytz Oct 5, 2019

ZackerySpytz Oct 5, 2019

ZackerySpytz Oct 5, 2019

ZackerySpytz Oct 5, 2019

vsajip Oct 5, 2019

vsajip Oct 5, 2019

zooba left a comment

vsajip commented Oct 7, 2019

ericsnowcurrently left a comment

ericsnowcurrently Oct 8, 2019

vsajip Oct 8, 2019

ericsnowcurrently Oct 8, 2019

vsajip Oct 8, 2019

vsajip commented Oct 8, 2019 •

edited

Loading

vsajip commented Oct 8, 2019

miss-islington commented Oct 9, 2019

miss-islington commented Oct 9, 2019

bedevere-bot commented Oct 9, 2019

miss-islington commented Oct 9, 2019

miss-islington commented Oct 9, 2019

bedevere-bot commented Oct 9, 2019

doko42 commented Oct 9, 2019

vsajip commented Oct 9, 2019

bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16589

bpo-38368: Added fix for ctypes crash when handling arrays in structs… #16589

Conversation

vsajip commented Oct 4, 2019 • edited by bedevere-bot Loading

vsajip commented Oct 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zooba left a comment

Choose a reason for hiding this comment

vsajip commented Oct 7, 2019

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsajip commented Oct 8, 2019 • edited Loading

vsajip commented Oct 8, 2019

miss-islington commented Oct 9, 2019

miss-islington commented Oct 9, 2019

bedevere-bot commented Oct 9, 2019

miss-islington commented Oct 9, 2019

miss-islington commented Oct 9, 2019

bedevere-bot commented Oct 9, 2019

doko42 commented Oct 9, 2019

vsajip commented Oct 9, 2019

vsajip commented Oct 4, 2019 •

edited by bedevere-bot

Loading

vsajip commented Oct 8, 2019 •

edited

Loading