Skip to content

bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module #31759

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
merged 4 commits into from
Mar 8, 2022
Merged

bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module #31759

merged 4 commits into from
Mar 8, 2022

Conversation

tiran
Copy link
Member

@tiran tiran commented Mar 8, 2022 •
edited by miss-islington
Loading

https://bugs.python.org/issue23325

Automerge-Triggered-By: GH:tiran

@tiran tiran added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Mar 8, 2022
@tiran tiran force-pushed the bpo-23325-signal-int-cmp branch from 1f535e0 to 3438104 Compare March 8, 2022 11:33
serhiy-storchaka
serhiy-storchaka reviewed Mar 8, 2022
View reviewed changes
Modules/signalmodule.c Outdated
@@ -1893,7 +1911,15 @@ PyErr_SetInterruptEx(int signum)

signal_state_t *state = &signal_global_state;
PyObject *func = get_handler(signum);
if (func != state->ignore_handler && func != state->default_handler) {
int is_ign = PyObject_RichCompareBool(func, state->ignore_handler, Py_EQ);
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay to call a Python code in PyErr_SetInterruptEx()?

If func is a custom callable, it can have an __eq__ method which can call an arbitrary Python code here.

It may be safer to use PyLong_Check() + PyLong_GetLongAndOverflow().

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

I took a different approach and added a helper function. We can safely assume that the internal handler objects are always exact longs. Comparison between exact longs should never fail.

@tiran tiran force-pushed the bpo-23325-signal-int-cmp branch from 67876c2 to cc4df05 Compare March 8, 2022 16:28
@miss-islington miss-islington merged commit c8a47e7 into python:main Mar 8, 2022
@miss-islington
Copy link
Contributor

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @tiran, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker c8a47e76a391c8818bf10a282cdcd3bb5c23ebf6 3.9

@miss-islington miss-islington self-assigned this Mar 8, 2022
@bedevere-bot
Copy link

GH-31768 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Mar 8, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 8, 2022
@tiran @miss-islington
bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (p…
4f7b447 
…ythonGH-31759)

(cherry picked from commit c8a47e7)

Co-authored-by: Christian Heimes <[email protected]>
miss-islington added a commit that referenced this pull request Mar 8, 2022
@miss-islington @tiran
bpo-23325: Fix SIG_IGN and SIG_DFL int comparison in signal module (G…
95b001f 
…H-31759)

(cherry picked from commit c8a47e7)

Co-authored-by: Christian Heimes <[email protected]>
@tiran tiran mentioned this pull request Jun 9, 2022
Merged
@serhiy-storchaka serhiy-storchaka mentioned this pull request Apr 10, 2022
Closed
@ZeroIntensity ZeroIntensity removed the needs backport to 3.9 only security fixes label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

@miss-islington miss-islington

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tiran @miss-islington @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni @ZeroIntensity