gh-107940: Only reuse the pointer object when it matches the _type_ of the container

[ambv]

Closes#107940. Also, solves a related yet undiscovered issue where an array of pointers reuses the array's memory for the pointer objects.

• Issue: incompatible behaviour between 3.12.0rc1 and previous versions in ctypes #107940

[ambv]

Skipping news as this is a fix for a yet unreleased regression.

[ambv]

cc @code-of-kpp

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

[code-of-kpp]

Do you think it might be useful to call PyErr_SetString here?

[ambv]

I did what other code in _ctypes.c did for this usage, which wasn't to set the error string.

[code-of-kpp]

Oh, yes, PyObject_IsInstance will do it if there's an error

[code-of-kpp]

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.

[code-of-kpp]

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)] ispp

so 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

[code-of-kpp]

I definitely got something weird, that I can't quickly figure out:

deftest_pointer_set_contents(self):
classStruct(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) # worksself.assertEqual(pp[0].contents.a, 32) # worksself.assertEqual(pp.contents[0].a, 32) # worksself.assertEqual(pp.contents.contents.a, 32) # fails, wat, holds 23self.assertEqual(p.contents.a, 32) # fails, wat, holds 23

[ambv]

I can't repro your result. For me pp.contents.contents itself leads to the assertion failure in Pointer_get_contents. I'm looking into further.

[code-of-kpp]

Very interesting. If you're OK with this, I can spend some time this week investigating too.

[ambv]

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.

[code-of-kpp]

I have a partial solution:

ptr2ptr= (CDataObject*) PyDict_GetItemString(keep, "1");
if (ptr2ptr==NULL) {
PyErr_SetString(PyExc_ValueError,
"Unexpected NULL pointer in _objects");
returnNULL;
}
// if we need to return a pointer, an array or a structure,// Pointer_item will do the right thingif (
PyCStructTypeObject_Check(stgdict->proto) ||PyCArrayTypeObject_Check(stgdict->proto) ||PyCPointerTypeObject_Check(stgdict->proto)
) {
returnPointer_item(self, 0);
} elseif ( PyUnicode_Check(stgdict->proto)
&& (strchr("sPzUZXO", PyUnicode_AsUTF8(stgdict->proto)[0]))) {
// simple pointer types, c_void_p, c_wchar_p, BSTR, ... */returnPointer_item(self, 0);
}

So, basically, if we are about to return Pointer, Structure or an Array, back off to the same thing as [0] will do.

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.
All other tests pass.
What do you think?