Skip to content

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

Merged
markshannon merged 3 commits into
python:masterfrom
vstinner:check_slot
Jan 27, 2021
Merged

bpo-42979: Enhance abstract.c assertions checking slot result#24352
markshannon merged 3 commits into
python:masterfrom
vstinner:check_slot

Conversation

@vstinner

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

Copy link
Copy Markdown
Member

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

vstinner commented Jan 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

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
Comment thread Include/cpython/abstract.h Outdated

@markshannon markshannon Jan 27, 2021

Copy link
Copy Markdown
Member

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/

@vstinner vstinner Jan 27, 2021

Copy link
Copy Markdown
Member Author

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

markshannon commented Jan 27, 2021

Copy link
Copy Markdown
Member

LGTM, apart from the API creep.

@vstinner

vstinner commented Jan 27, 2021

Copy link
Copy Markdown
Member Author

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