Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

code-of-kpp
Copy link
Contributor

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

@bedevere-bot bedevere-bot mentioned this pull request Aug 26, 2023
Open
@ambv
Copy link
Contributor

ambv commented Aug 26, 2023

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.

@code-of-kpp
Copy link
Contributor Author

code-of-kpp commented Aug 26, 2023 •
edited
Loading

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 code-of-kpp force-pushed the test-p-set-contents branch 3 times, most recently from 5cf867d to a84b664 Compare August 26, 2023 19:18
@code-of-kpp
Copy link
Contributor Author

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 code-of-kpp force-pushed the test-p-set-contents branch 2 times, most recently from 3e346a8 to 5dbb4ae Compare August 27, 2023 22:51
@code-of-kpp code-of-kpp force-pushed the test-p-set-contents branch 3 times, most recently from d44a269 to b6e92ba Compare September 7, 2023 00:16
@s1kpp
pythongh-46376: add (failing) tests for ctypes/pointer/cast/set_contents
0116633 
Previous attempt to fix pythongh-46376 was incomplete and
overall it didn't succeed, and was reverted in 3.11.
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.
@code-of-kpp code-of-kpp force-pushed the test-p-set-contents branch from b6e92ba to 0116633 Compare July 13, 2024 13:14
@encukou
Copy link
Member

encukou commented Jul 18, 2024

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.

picnixz
picnixz reviewed Jul 22, 2024
View reviewed changes
Lib/test/test_ctypes/test_arrays.py
@@ -1,8 +1,10 @@
import gc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this one after ctypes (for sorting imports).

Lib/test/test_ctypes/test_cast.py
class Struct(Structure):
_fields_ = [("a", POINTER(c_int))]

x = Struct(a=pointer(w.setdefault(23,c_int(23))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
x = Struct(a=pointer(w.setdefault(23,c_int(23))))
x = Struct(a=pointer(w.setdefault(23, c_int(23))))

Lib/test/test_ctypes/test_keeprefs.py
@@ -98,6 +100,29 @@ def test_p_cint(self):
x = pointer(i)
self.assertEqual(x._objects, {'1': i})

@unittest.expectedFailure # gh-46376, gh-107131, gh-107940, gh-108222
def test_pp_ownership(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to instead have a huge expectedFailure, to correctly detect where it is failing instead? Because among the assert* calls, I don't know which of these is the one that should actually fail (same question for all the other expected failures).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@picnixz picnixz picnixz left review comments

Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ctypes pointer not always keeping target alive
6 participants
@code-of-kpp @ambv @encukou @picnixz @bedevere-bot @s1kpp