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

Remove old ctypes callback workaround: creating the first instance of a callback #79704

Closed
vstinner opened this issue Dec 18, 2018 · 7 comments
Closed
Labels
3.8 only security fixes stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

BPO 35523
Nosy @vstinner, @ned-deily, @ideasman42
PRs
  • bpo-35523: Remove ctypes callback workaround #11211
  • 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 = None
    closed_at = <Date 2018-12-18.14:10:26.162>
    created_at = <Date 2018-12-18.12:04:01.344>
    labels = ['3.8', 'library']
    title = 'Remove old ctypes callback workaround: creating the first instance of a callback'
    updated_at = <Date 2021-01-25.19:31:11.357>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-01-25.19:31:11.357>
    actor = 'ned.deily'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-12-18.14:10:26.162>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2018-12-18.12:04:01.344>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35523
    keywords = ['patch']
    message_count = 7.0
    messages = ['332054', '332059', '332061', '385596', '385597', '385613', '385650']
    nosy_count = 3.0
    nosy_names = ['vstinner', 'ned.deily', 'ideasman42']
    pr_nums = ['11211']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue35523'
    versions = ['Python 3.8']

    @vstinner
    Copy link
    Member Author

    ctypes._reset_cache() contains the following code:

    # XXX for whatever reasons, creating the first instance of a callback
    # function is needed for the unittests on Win64 to succeed.  This MAY
    # be a compiler bug, since the problem occurs only when _ctypes is
    # compiled with the MS SDK compiler.  Or an uninitialized variable?
    CFUNCTYPE(c_int)(lambda: None)
    

    This code has been added 11 years ago:

    commit 674e938
    Author: Thomas Heller <theller@ctypes.org>
    Date: Fri Aug 31 13:06:44 2007 +0000

    Add a workaround for a strange bug on win64, when _ctypes is compiled
    with the SDK compiler.  This should fix the failing
    Lib\ctypes\test\test_as_parameter.py test.
    
    diff --git a/Lib/ctypes/__init__.py b/Lib/ctypes/__init__.py
    index cdf6d36e47..f55d194b8f 100644
    --- a/Lib/ctypes/__init__.py
    +++ b/Lib/ctypes/__init__.py
    @@ -535,3 +535,9 @@ for kind in [c_ushort, c_uint, c_ulong, c_ulonglong]:
         elif sizeof(kind) == 4: c_uint32 = kind
         elif sizeof(kind) == 8: c_uint64 = kind
     del(kind)
    +
    +# XXX for whatever reasons, creating the first instance of a callback
    +# function is needed for the unittests on Win64 to succeed.  This MAY
    +# be a compiler bug, since the problem occurs only when _ctypes is
    +# compiled with the MS SDK compiler.  Or an uninitialized variable?
    +CFUNCTYPE(c_int)(lambda: None)

    --

    This call is removed from Fedora package by the following patch:

    https://src.fedoraproject.org/rpms/python3/blob/master/f/00155-avoid-ctypes-thunks.patch

    Extract of Fedora python3.spec:

    \bpo-00155 #
    # Avoid allocating thunks in ctypes unless absolutely necessary, to avoid
    # generating SELinux denials on "import ctypes" and "import uuid" when
    # embedding Python within httpd
    # See https://bugzilla.redhat.com/show_bug.cgi?id=814391
    Patch155: 00155-avoid-ctypes-thunks.patch

    The patch has been added 6 years ago in Fedora:

    commit 8a28107df1670a03a12cf6a7787160f103d8d8c8
    Author: David Malcolm <dmalcolm@redhat.com>
    Date: Fri Apr 20 15:28:39 2012 -0400

    3.2.3-4: avoid allocating thunks in ctypes unless absolutely necessary (patch 155; rhbz#814391)
    
    * Fri Apr 20 2012 David Malcolm <dmalcolm@redhat.com> - 3.2.3-4
    - avoid allocating thunks in ctypes unless absolutely necessary, to avoid
    generating SELinux denials on "import ctypes" and "import uuid" when embedding
    Python within httpd (patch 155; rhbz#814391)
    

    https://src.fedoraproject.org/rpms/python3/c/8a28107df1670a03a12cf6a7787160f103d8d8c8?branch=master

    --

    I don't understand the purpose of the workaround and ctypes is working well on Fedora. I propose to also remove the workaround in the master branch.

    In case of doubt, I prefer to keep the workaround in Python 3.7.

    Attached PR removes the workaround.

    @vstinner vstinner added 3.8 only security fixes stdlib Python modules in the Lib dir labels Dec 18, 2018
    @vstinner vstinner changed the title Remove ctypes old workaround: creating the first instance of a callback Remove old ctypes callback workaround: creating the first instance of a callback Dec 18, 2018
    @vstinner
    Copy link
    Member Author

    New changeset e6b247c by Victor Stinner in branch 'master':
    bpo-35523: Remove ctypes callback workaround (GH-11211)
    e6b247c

    @vstinner
    Copy link
    Member Author

    I will keep an eye on buildbots next days.

    @ideasman42
    Copy link
    Mannequin

    ideasman42 mannequin commented Jan 25, 2021

    Could this fix be applied to Python 3.7x?

    Currently the visual effects platform for 2021 is using Python 3.7.x, see: https://vfxplatform.com

    Which means anyone using the VFX platform can run into this bug.

    @ideasman42
    Copy link
    Mannequin

    ideasman42 mannequin commented Jan 25, 2021

    Note, this bug is something we ran into in Blender, see: https://developer.blender.org/T84752

    @vstinner
    Copy link
    Member Author

    Ned Deily (Python 3.7 release manager): do you want an exception to backport a bugfix?

    Campbell: Python 3.7 no longer accept bug fixes, only security fixes.
    https://devguide.python.org/#status-of-python-branches

    As I wrote previously, I'm not comfortable to make such change in a 3.7.x minor release, even if we didn't get any bug report since this change was done in Python 3.8.

    @ned-deily
    Copy link
    Member

    It is certainly not good that some of the vfx users are seeing a crash. On the other hand, at this stage of Python 3.7's life cycle only security-related fixes are accepted as a primary goal is to keep 3.7 as stable and unchanged as possible for those users still using it. We always recommend using a fully supported version, today either 3.8.7 (for several more months) or 3.9.1. It is unfortunate that the Fedora patch wasn't applied upstream earlier when it would have been exposed across all platforms. If we could be more certain that applying it now would have no side effects on other platforms, it might be an easier call. As long as the vfx project is building and supporting its own copies of Python 3.7, I think it would be better for them to carry the Fedora patch locally if they want it until they migrate to a newer version of Python.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants