Skip to content

/ cpython

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

Jump to bottom

bpo-42979: Enhance abstract.c assertions checking slot result #24352

Merged
merged 3 commits into from Jan 27, 2021
Merged

bpo-42979: Enhance abstract.c assertions checking slot result #24352

merged 3 commits into from Jan 27, 2021
+126 −51

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Jan 27, 2021 •
edited by bedevere-bot

Add _Py_CheckSlotResult() function which fails with a fatal error if
a slot function succeeded with an exception set or failed with no
exception set: write the slot name, the type name and the current
exception (if an exception is set).

https://bugs.python.org/issue42979
@vstinner vstinner added the skip news label Jan 27, 2021
@vstinner vstinner requested a review from markshannon as a code owner Jan 27, 2021
@vstinner
Copy link
Member Author

@vstinner vstinner commented Jan 27, 2021 •
edited

Example which tries to mimick bpo-42979 by injecting PyErr_SetString(PyExc_ValueError, "bug") before PyType_Ready() is called in zoneinfo exec function:

Fatal Python error: _Py_CheckSlotResult: Slot __getitem__ of type dict succeeded with an exception set
Python runtime state: initialized
ValueError: bug

Current thread 0x00007ff43e1b1740 (most recent call first):
  File "<frozen importlib._bootstrap>", line 241 in _call_with_frames_removed
  File "<frozen importlib._bootstrap_external>", line 1134 in exec_module
  File "<frozen importlib._bootstrap>", line 698 in _load_unlocked
  File "<frozen importlib._bootstrap>", line 729 in _load
  File "/home/vstinner/python/master/./setup.py", line 620 in check_extension_import
  File "/home/vstinner/python/master/./setup.py", line 475 in build_extensions
  File "/home/vstinner/python/master/Lib/distutils/command/build_ext.py", line 340 in run
  File "/home/vstinner/python/master/Lib/distutils/dist.py", line 985 in run_command
  File "/home/vstinner/python/master/Lib/distutils/cmd.py", line 313 in run_command
  File "/home/vstinner/python/master/Lib/distutils/command/build.py", line 135 in run
  File "/home/vstinner/python/master/Lib/distutils/dist.py", line 985 in run_command
  File "/home/vstinner/python/master/Lib/distutils/dist.py", line 966 in run_commands
  File "/home/vstinner/python/master/Lib/distutils/core.py", line 148 in setup
  File "/home/vstinner/python/master/./setup.py", line 2632 in main
  File "/home/vstinner/python/master/./setup.py", line 2662 in <module>

Injected bug:

diff --git a/Modules/_zoneinfo.c b/Modules/_zoneinfo.c
index d0c462fb86..f0332b9b33 100644
--- a/Modules/_zoneinfo.c
+++ b/Modules/_zoneinfo.c
@@ -2633,6 +2633,7 @@ zoneinfomodule_exec(PyObject *m)
         goto error;
     }
     PyZoneInfo_ZoneInfoType.tp_base = PyDateTimeAPI->TZInfoType;
+    PyErr_SetString(PyExc_ValueError, "bug");
     if (PyType_Ready(&PyZoneInfo_ZoneInfoType) < 0) {
         goto error;
     }
@markshannon
markshannon reviewed Jan 27, 2021
View changes
Include/cpython/abstract.h Outdated
@@ -39,6 +39,12 @@ PyAPI_FUNC(PyObject *) _Py_CheckFunctionResult(
PyObject *result,
const char *where);

// Usage: assert(_Py_CheckSlotResult(obj, "__getitem__", result != NULL)));
PyAPI_FUNC(int) _Py_CheckSlotResult(

This comment has been minimized.

Sign in to view
@markshannon

markshannon Jan 27, 2021
Contributor

Why an API function?
You know that you will regret it later 😉 https://www.python.org/dev/peps/pep-0620/

This comment has been minimized.

Sign in to view
@vstinner

vstinner Jan 27, 2021
Author Member

Oh, let me see that PEP... ah yes ;-) I wanted to define it close to _Py_CheckFunctionResult() definition, but you're right that it's a bad idea to export the symbol.
@markshannon
Copy link
Contributor

@markshannon markshannon commented Jan 27, 2021

LGTM, apart from the API creep.
@vstinner vstinner force-pushed the vstinner:check_slot branch from 22c7479 to 3afbbaf Jan 27, 2021
@vstinner
Copy link
Member Author

@vstinner vstinner commented Jan 27, 2021

the API creep

Fixed: I moved the function definition to the internal C API. I also added an unit test.

Note: this change has no impact on a release build (when Python is built with NDEBUG macro defined).
@vstinner
bpo-42979: Enhance abstract.c assertions checking slot result
Loading status checks…
014163a 
Add _Py_CheckSlotResult() function which fails with a fatal error if
a slot function succeeded with an exception set or failed with no
exception set: write the slot name, the type name and the current
exception (if an exception is set).
@vstinner vstinner force-pushed the vstinner:check_slot branch from 3afbbaf to 014163a Jan 27, 2021
vstinner added 2 commits Jan 27, 2021
@vstinner
Cleanup
Loading status checks…
97c0df2
@vstinner
Remove incorrect comments
Loading status checks…
b223b55
@markshannon markshannon merged commit c9b8e9c into python:master Jan 27, 2021
10 checks passed
10 checks passed
Check for source changes
Details
Check if generated files are up to date
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20210127.14 succeeded
Details
Travis CI - Pull Request Build Passed
Details
bedevere/issue-number Issue number 42979 found
Details
bedevere/news "skip news" label found
@vstinner vstinner deleted the vstinner:check_slot branch Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markshannon markshannon

Assignees
No one assigned
Labels
CLA signed skip news
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@vstinner @markshannon @the-knights-who-say-ni @bedevere-bot