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

_zoneinfo: zoneinfomodule_exec() doesn't check for PyDateTime_IMPORT failure #87145

Closed
vstinner opened this issue Jan 20, 2021 · 9 comments
Closed
Labels
3.10 only security fixes extension-modules C modules in the Modules dir

Comments

@vstinner
Copy link
Member

BPO 42979
Nosy @vstinner, @markshannon, @miss-islington, @shihai1991
PRs
  • bpo-42979: Check the import of datetime's capi succeeded or not in zoneinfo. #24333
  • [3.9] bpo-42979: _zoneinfo exec function checks for PyDateTime_IMPORT failure (GH-24333) #24351
  • bpo-42979: Enhance abstract.c assertions checking slot result #24352
  • bpo-42979: Use _Py_CheckSlotResult() to check slots result #24356
  • 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 2021-01-27.10:28:15.795>
    created_at = <Date 2021-01-20.17:26:06.255>
    labels = ['extension-modules', '3.10']
    title = "_zoneinfo: zoneinfomodule_exec() doesn't check for PyDateTime_IMPORT failure"
    updated_at = <Date 2021-01-29.15:53:36.884>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2021-01-29.15:53:36.884>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-01-27.10:28:15.795>
    closer = 'vstinner'
    components = ['Extension Modules']
    creation = <Date 2021-01-20.17:26:06.255>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42979
    keywords = ['patch']
    message_count = 9.0
    messages = ['385355', '385571', '385615', '385754', '385755', '385757', '385766', '385787', '385915']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'Mark.Shannon', 'miss-islington', 'shihai1991']
    pr_nums = ['24333', '24351', '24352', '24356']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42979'
    versions = ['Python 3.10']

    @vstinner
    Copy link
    Member Author

    _zoneinfo starts with:

    static int
    zoneinfomodule_exec(PyObject *m)
    {
        PyDateTime_IMPORT;

    with:

    #define PyDateTime_CAPSULE_NAME "datetime.datetime_CAPI"
    #define PyDateTime_IMPORT \
        PyDateTimeAPI = (PyDateTime_CAPI *)PyCapsule_Import(PyDateTime_CAPSULE_NAME, 0)

    If PyCapsule_Import() fails, zoneinfomodule_exec() returns 0 (success) with an exception raised.

    It should check if the import succeeded or not. Concrete example on AIX where datetime cannot be imported:
    https://bugs.python.org/issue42604#msg385347

    "./python setup.py build" fails with:

    Assertion failed: (item != NULL) ^ (PyErr_Occurred() != NULL), file Objects/abstract.c, line 163

    --

    By the way, the import machinery should raise a SystemError if a module exec function raises an exception *and* reports a success: see _Py_CheckFunctionResult().

    @vstinner vstinner added 3.10 only security fixes extension-modules C modules in the Modules dir labels Jan 20, 2021
    @shihai1991
    Copy link
    Member

    If PyCapsule_Import() fails, zoneinfomodule_exec() returns 0 (success) with an exception raised.

    Why not return -1 directly when PyCapsule_Import() fails?
    It looks like more simpler. Do I miss something?

    @vstinner
    Copy link
    Member Author

    Why not return -1 directly when PyCapsule_Import() fails?

    That would work.

    I opened an issue since there is a bug.

    @vstinner
    Copy link
    Member Author

    New changeset eeb701a by Hai Shi in branch 'master':
    bpo-42979: _zoneinfo exec function checks for PyDateTime_IMPORT failure (GH-24333)
    eeb701a

    @vstinner
    Copy link
    Member Author

    Thanks for the fix Hai Shi!

    By the way, the import machinery should raise a SystemError if a module exec function raises an exception *and* reports a success: see _Py_CheckFunctionResult().

    It's already the case. Example:

    >>> import _zoneinfo
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    SystemError: execution of module _zoneinfo raised unreported exception

    With this bug:

    diff --git a/Modules/_zoneinfo.c b/Modules/_zoneinfo.c
    index d0c462fb86..fc564b9587 100644
    --- a/Modules/_zoneinfo.c
    +++ b/Modules/_zoneinfo.c
    @@ -2683,6 +2683,7 @@ zoneinfomodule_exec(PyObject *m)
             goto error;
         }
     
    +    PyErr_SetString(PyExc_Exception, "BUG");
         return 0;
     
     error:

    @vstinner
    Copy link
    Member Author

    New changeset 895591c by Miss Islington (bot) in branch '3.9':
    bpo-42979: _zoneinfo exec function checks for PyDateTime_IMPORT failure (GH-24333) (GH-24351)
    895591c

    @shihai1991
    Copy link
    Member

    It's already the case. Example

    Nice, interesting case.

    @markshannon
    Copy link
    Member

    New changeset c9b8e9c by Victor Stinner in branch 'master':
    bpo-42979: Enhance abstract.c assertions checking slot result (GH-24352)
    c9b8e9c

    @vstinner
    Copy link
    Member Author

    New changeset a619263 by Victor Stinner in branch 'master':
    bpo-42979: Use _Py_CheckSlotResult() to check slots result (GH-24356)
    a619263

    @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.10 only security fixes extension-modules C modules in the Modules dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants