◐ Shell
reader mode source ↗
Skip to content

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

Open
code-of-kpp wants to merge 1 commit into
python:mainfrom
code-of-kpp:test-p-set-contents
Open

gh-46376: add (failing) tests for ctypes/pointer/cast/set_contents#108519
code-of-kpp wants to merge 1 commit into
python:mainfrom
code-of-kpp:test-p-set-contents

Conversation

@code-of-kpp

@code-of-kpp code-of-kpp commented Aug 26, 2023

Copy link
Copy Markdown
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

ambv commented Aug 26, 2023

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
Contributor Author

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

code-of-kpp commented Aug 27, 2023

Copy link
Copy Markdown
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

encukou commented Jul 18, 2024

Copy link
Copy Markdown
Member

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.

@github-actions

github-actions Bot commented Apr 13, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

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