Skip to content
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.

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

ctypes pointer not always keeping target alive #46376

Open
arigo mannequin opened this issue Feb 15, 2008 · 11 comments · May be fixed by #108519
Open

ctypes pointer not always keeping target alive #46376

arigo mannequin opened this issue Feb 15, 2008 · 11 comments · May be fixed by #108519
Assignees
Labels
extension-modules C modules in the Modules dir topic-ctypes type-bug An unexpected behavior, bug, or error

Comments

@arigo
Copy link
Mannequin

arigo mannequin commented Feb 15, 2008

BPO 2123
Nosy @arigo, @theller, @bitdancer
Files
  • x.py: failing test or bogus test?
  • y.py
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/theller'
    closed_at = None
    created_at = <Date 2008-02-15.14:24:58.157>
    labels = ['extension-modules', 'type-bug']
    title = 'ctypes pointer not always keeping target alive'
    updated_at = <Date 2013-07-10.10:14:10.589>
    user = 'https://github.com/arigo'

    bugs.python.org fields:

    activity = <Date 2013-07-10.10:14:10.589>
    actor = 'christian.heimes'
    assignee = 'theller'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules']
    creation = <Date 2008-02-15.14:24:58.157>
    creator = 'arigo'
    dependencies = []
    files = ['9436', '9438']
    hgrepos = []
    issue_num = 2123
    keywords = []
    message_count = 5.0
    messages = ['62429', '62436', '62453', '84117', '84218']
    nosy_count = 3.0
    nosy_names = ['arigo', 'theller', 'r.david.murray']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'languishing'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue2123'
    versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

    Linked PRs

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Feb 15, 2008

    It's hard to tell for sure, given the lack of precise definition, but I
    believe that the attached piece of code "should" work. What it does is
    make p1 point to c_long(20). So ctypes should probably keep the
    c_long(20) alive as long as p1 is alive (and not further modified).
    This test shows that the c_long(20) gets freed instead, making the
    p1.contents reference garbage.

    @arigo arigo mannequin added the extension-modules C modules in the Modules dir label Feb 15, 2008
    @theller
    Copy link

    theller commented Feb 15, 2008

    May I ask: do you have a real use case for this, or is it a carefully
    constructed example?

    Of course I take all the blame for not defining/documenting this
    stuff. My current view is this:

    Python code C code
    ======================= ================

    ptr = POINTER(c_long)()         int *ptr = NULL;
    x = c_long(42)                  int x = 42;
    
    ptr.contents = x                ptr = &x;
    
    a = ptr[0]                      int a = *ptr;
    b = ptr[n]                      int b = ptr[n];

    Assigning to .contents changes 'where the pointer points to'.
    __setitem__ changes the pointed to memory location; __getitem__
    retrieves the pointed to memory location.

    Having said that, it is no longer clear to me what reading the
    .contents attribute should mean. Would making the .contents attribute
    write-only help - is it impossible to construct this 'bug' without
    assigning to .contents?

    @theller theller self-assigned this Feb 15, 2008
    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Feb 16, 2008

    We're finding such bugs because we are trying to reimplement ctypes in PyPy.

    I guess your last question was "is it impossible to construct this 'bug'
    without *reading* .contents?". The answer is that it doesn't change
    much; you can replace all the reads from .contents with reads from [0],
    and still see the issue. See attached y.py, where I've put the
    equivalent C code in comments.

    @bitdancer
    Copy link
    Member

    Thomas, do you accept this as a bug or should the issue be closed as
    invalid?

    @bitdancer bitdancer added the type-bug An unexpected behavior, bug, or error label Mar 24, 2009
    @theller
    Copy link

    theller commented Mar 26, 2009

    I accept this as a bug; however I don't have time now to work on it.

    @tiran tiran added the stale Stale PR or inactive for long period of time. label Jul 10, 2013
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @ambv
    Copy link
    Contributor

    ambv commented Jul 22, 2023

    This looks like it works as expected at least as of Python 3.7 where c_long(20) is indeed preserved and p1.contents removes 20 as expected.

    @code-of-kpp
    Copy link
    Contributor

    Original examples would pass, but only because of gc delay.

    Here's fun way to demonstrate that the problem still exists (and can be converted to a test):

    import ctypes
    import weakref
    
    p1 = ctypes.pointer(ctypes.c_long(10))
    w = weakref.WeakValueDictionary()
    ctypes.pointer(p1).contents.contents = w.setdefault(
        1, ctypes.c_long(-1),
    )
    assert p1.contents.value == -1
    assert 1 in w

    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Jul 23, 2023
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Jul 23, 2023
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Jul 24, 2023
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Jul 24, 2023
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Jul 24, 2023
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Jul 25, 2023
    ambv pushed a commit to ambv/cpython that referenced this issue Jul 31, 2023
    pythonGH-107131)
    
    (cherry picked from commit 08447b5)
    
    Co-authored-by: Konstantin <kpp.live+github@gmail.com>
    ambv pushed a commit to ambv/cpython that referenced this issue Jul 31, 2023
    pythonGH-107131)
    
    (cherry picked from commit 08447b5)
    
    Co-authored-by: Konstantin <kpp.live+github@gmail.com>
    ambv added a commit that referenced this issue Jul 31, 2023
    …107131) (#107487)
    
    (cherry picked from commit 08447b5)
    
    Co-authored-by: Konstantin <kpp.live+github@gmail.com>
    ambv added a commit that referenced this issue Jul 31, 2023
    …107131) (#107488)
    
    (cherry picked from commit 08447b5)
    
    Co-authored-by: Konstantin <kpp.live+github@gmail.com>
    @ambv
    Copy link
    Contributor

    ambv commented Jul 31, 2023

    This ancient issue is now fixed. Thanks, Konstantin! ✨ 🍰 ✨

    ambv added a commit to ambv/cpython that referenced this issue Aug 21, 2023
    ambv added a commit to ambv/cpython that referenced this issue Aug 22, 2023
    ambv added a commit to ambv/cpython that referenced this issue Aug 24, 2023
    ambv added a commit that referenced this issue Aug 24, 2023
    …pes (GH-107131) (GH-107488)" (#108412)
    
    This reverts commit 57f27e4.
    
    The fix caused gh-107940. Until we have a bulletproof fix for that, the 3.11 backport needs to be reverted to make way for 3.11.5.
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Aug 26, 2023
    Previous attempt to fix pythongh-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.
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Aug 26, 2023
    Previous attempt to fix pythongh-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.
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Aug 26, 2023
    Previous attempt to fix pythongh-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.
    @code-of-kpp
    Copy link
    Contributor

    This should be re-opened, as fix in 3.11 was reverted and the one in 3.12/3.13/main isn't working always

    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Aug 26, 2023
    Previous attempt to fix pythongh-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.
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Aug 26, 2023
    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 pushed a commit to code-of-kpp/cpython that referenced this issue Aug 26, 2023
    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 pushed a commit to code-of-kpp/cpython that referenced this issue Aug 27, 2023
    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 pushed a commit to code-of-kpp/cpython that referenced this issue Aug 27, 2023
    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.
    @vstinner
    Copy link
    Member

    This should be re-opened, as fix in 3.11 was reverted and the one in 3.12/3.13/main isn't working always

    Right, see issue #107940 which is a Python 3.12 regression.

    In the 3.12 branch, I confirm that the commit 54aaaad introduced the regression: issue #46376 of PR #107487 (backport of PR #107131).

    vstinner added a commit to vstinner/cpython that referenced this issue Aug 30, 2023
    …es (python#107131)"
    
    This reverts commit 08447b5.
    
    Revert also _ctypes.c changes of the PyDict_ContainsString() change,
    commit 6726626.
    @code-of-kpp
    Copy link
    Contributor

    Please, take a look at
    #108519

    ambv pushed a commit that referenced this issue Sep 4, 2023
    #108688)
    
    This reverts commit 08447b5.
    
    Revert also _ctypes.c changes of the PyDict_ContainsString() change,
    commit 6726626.
    @ambv ambv reopened this Sep 4, 2023
    ambv added a commit to ambv/cpython that referenced this issue Sep 4, 2023
    hugovk pushed a commit to hugovk/cpython that referenced this issue Sep 4, 2023
    …es (#1… (python#108688)
    
    This reverts commit 08447b5.
    
    Revert also _ctypes.c changes of the PyDict_ContainsString() change,
    commit 6726626.
    ambv added a commit that referenced this issue Sep 4, 2023
    …pes (GH-107131) (GH-107487)" (#108864)
    
    This reverts commit 54aaaad.
    
    Co-authored-by: T. Wouters <thomas@python.org>
    code-of-kpp pushed a commit to code-of-kpp/cpython that referenced this issue Sep 6, 2023
    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 pushed a commit to code-of-kpp/cpython that referenced this issue Sep 6, 2023
    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 pushed a commit to code-of-kpp/cpython that referenced this issue Sep 7, 2023
    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.
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir topic-ctypes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    Successfully merging a pull request may close this issue.

    7 participants