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

[vstinner]

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]

_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().

[shihai1991]

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]

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

That would work.

I opened an issue since there is a bug.

[vstinner]

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

[vstinner]

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]

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]

It's already the case. Example

Nice, interesting case.

[markshannon]

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

[vstinner]

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