Skip to content

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

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.

Sign up for GitHub

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
Merged
markshannon merged 3 commits into from
Jan 27, 2021
Merged

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

markshannon merged 3 commits into from
Jan 27, 2021

Conversation

@vstinner
Copy link
Member

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

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 requested a review from markshannon as a code owner January 27, 2021 11:04
@vstinner
Copy link
Member Author

vstinner commented Jan 27, 2021 •
edited
Loading

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 reviewed changes
Include/cpython/abstract.h Outdated
Copy link
Member

@markshannon markshannon Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@vstinner vstinner Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Member

markshannon commented Jan 27, 2021

LGTM, apart from the API creep.

@vstinner
Copy link
Member Author

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
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
Cleanup
97c0df2
@vstinner
Remove incorrect comments
b223b55
@markshannon markshannon merged commit c9b8e9c into python:master Jan 27, 2021
@vstinner vstinner deleted the check_slot branch January 27, 2021 21:45
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@vstinner @adorilson
bpo-42979: Enhance abstract.c assertions checking slot result (python…
96f7d78 
…GH-24352)

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

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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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