gh-107940: Only reuse the pointer object when it matches the _type_ of the container
#108222
Conversation
|
Skipping news as this is a fix for a yet unreleased regression. |
|
cc @code-of-kpp |
|
Yep, there's a bug for sure, I had an intuition that if one would come up with some interesting way to cast things, we can break things, just couldn't come up with an example back then. |
| // `_objects` is shared between casts and the original. | ||
| int res = PyObject_IsInstance(ptr2ptr, stgdict->proto); | ||
| if (res == -1) { | ||
| return NULL; |
There was a problem hiding this comment.
Do you think it might be useful to call PyErr_SetString here?
There was a problem hiding this comment.
I did what other code in _ctypes.c did for this usage, which wasn't to set the error string.
There was a problem hiding this comment.
Oh, yes, PyObject_IsInstance will do it if there's an error
| ); // double-check that we are returning the same thing | ||
| Py_INCREF(ptr2ptr); | ||
| return (PyObject *) ptr2ptr; | ||
| return ptr2ptr; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If I understand correctly, we will fall down here if types are different, meaning that if someone will try to set contents of the contents, we'll have the same bug as we had originally - garbage in the memory. Let me check that/play with that a bit.
There was a problem hiding this comment.
cast(void *ptr, PyObject *src, *ctype does this:
result = (CDataObject *)_PyObject_CallNoArgs(ctype);
index = PyLong_FromVoidPtr((void *)src);
rc = PyDict_SetItem(result->b_objects, index, src);
memcpy(result->b_ptr, &ptr, sizeof(void *));
return (PyObject *)result;e.g., given how id currently works, in the test above:
cast(pp, POINTER(POINTER(c_int16)))._objects[id(pp)] is ppso it feels like the safe solution would be to call cast() in the
when if (res) { is false, the proper one would be to extact "unsafe"
part from cast() and call it directly
|
I definitely got something weird, that I can't quickly figure out: def test_pointer_set_contents(self):
class Struct(Structure):
_fields_ = [('a', c_int16)]
p = pointer(Struct(a=23))
self.assertIs(p._type_, Struct)
self.assertEqual(cast(p, POINTER(c_int16)).contents._type_, 'h')
pp = pointer(p)
self.assertIs(pp._type_, POINTER(Struct))
cast(pp, POINTER(POINTER(c_int16))).contents.contents = c_int16(32)
self.assertIs(p.contents, pp.contents.contents)
self.assertEqual(cast(p, POINTER(c_int16)).contents.value, 32)
self.assertEqual(p[0].a, 32) # works
self.assertEqual(pp[0].contents.a, 32) # works
self.assertEqual(pp.contents[0].a, 32) # works
self.assertEqual(pp.contents.contents.a, 32) # fails, wat, holds 23
self.assertEqual(p.contents.a, 32) # fails, wat, holds 23 |
|
I can't repro your result. For me |
|
Very interesting. If you're OK with this, I can spend some time this week investigating too. |
|
Sure. We might need to back out your original fix out of 3.11 until those rough edges are addressed as the next release is going to likely go out either today or tomorrow. |
|
I have a partial solution: ptr2ptr = (CDataObject*) PyDict_GetItemString(keep, "1");
if (ptr2ptr == NULL) {
PyErr_SetString(PyExc_ValueError,
"Unexpected NULL pointer in _objects");
return NULL;
}
// if we need to return a pointer, an array or a structure,
// Pointer_item will do the right thing
if (
PyCStructTypeObject_Check(stgdict->proto) ||
PyCArrayTypeObject_Check(stgdict->proto) ||
PyCPointerTypeObject_Check(stgdict->proto)
) {
return Pointer_item(self, 0);
} else if ( PyUnicode_Check(stgdict->proto)
&& (strchr("sPzUZXO", PyUnicode_AsUTF8(stgdict->proto)[0]))) {
// simple pointer types, c_void_p, c_wchar_p, BSTR, ... */
return Pointer_item(self, 0);
}So, basically, if we are about to return Pointer, Structure or an Array, back off to the same thing as It messes up a bit the garbage collection: replaced values are gonna be saved longer than needed. Still, I think it's a better solution, than what we have now in 3.11, where a memory leak can happen. |
…tainer Closes python#107940. Also, solves a related yet undiscovered issue where an array of pointers reuses the array's memory for the pointer objects.
Closes #107940. Also, solves a related yet undiscovered issue where an array of pointers reuses the array's memory for the pointer objects.