cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-46376: add (failing) tests for ctypes/pointer/cast/set_contents

Open code-of-kpp opened this issue 2 years ago • 3 comments

Previous attempt to fix gh-46376 was incomplete and overall it didn't succeed, and was reverted. However, we have discovered some dangerous issues with ctypes, that aren't fixed or documented anywhere. This commit adds tests (@expectedFailure) so at least developers are aware of situations where memory might be corrupted, leaked or when changing a pointer value might have no effect. Hopefully, we should be able to remove @expectedFailure in the future, with new shiny ctypes implementation or at least a bugfix.

/cc @ambv

code-of-kpp avatar Aug 26 '23 17:08 code-of-kpp

The previous fix was only reverted for 3.11 so we could release 3.11.5. The rest isn't in such a hurry so I hope we can fix it forward instead.

ambv avatar Aug 26 '23 17:08 ambv

I'm planning to fix it yes, but I'd like the tests to be merged first, if you agree that it's reasonable

code-of-kpp avatar Aug 26 '23 17:08 code-of-kpp

There are too many moving parts:

  1. my assumption that I can always find original object in "1"'s key in _objects is simply wrong, e.g., if the pointer is inside a structure, or it's one of the pointers in an array of pointers
  2. your attempt to fetch _objects directly also wouldn't work, for the same reason: you need to follow the chain of _b_base to find the shared _objects.
  3. it's not only Pointer_set_contents/Pointer_get_contents, but apparently Array_set.../Array_get, and probably PyCField_get/PyCField_set, that can create pointer clones.
  4. It's OK for different pointers (who have own memory address themselves), to point to the same object, but it's really difficult to ensure that they'll share _objects. ... and probably I'm missing something else

The biggest bug right now seems to be that set_contents might change the address of the pointer, not just the contents. That one, probably, we can fix. Still the solution for it wouldn't be what I originally proposed.

Another shortcut, would be to take my original idea, look into _objects["1"], make sure it's actually the same object and only in this case – return it. (so we'll have a partial fix).

code-of-kpp avatar Aug 27 '23 17:08 code-of-kpp

Hi, We currently don't use @expectedfailure, and so no one is looking at such tests. I don't think adding failing tests will bring much benefit over having the reproducers listed in an issue.

Would you mind adding only the tests that succeed? There's quite a few of those here.

encukou avatar Jul 18 '24 12:07 encukou