-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-99537: Use Py_SETREF() and Py_CLEAR() functions in C code #99538
Conversation
vstinner
commented
Nov 16, 2022
•
edited by bedevere-bot
Loading
edited by bedevere-bot
- Issue: Use Py_SETREF() macro to fix race conditions (potential race conditions) in C extensions #99537
Replace "Py_XDECREF(var); var=NULL;" with "Py_CLEAR(var);". Do the same when Py_DECREF() is used for similar code.
Fix race condition in usage code patterns: * Replace "Py_DECREF(var); var = new;" with "Py_SETREF(var, new);" * Replace "Py_XDECREF(var); var = new;" with "Py_XSETREF(var, new);" * Replace "Py_CLEAR(var); var = new;" with "Py_XSETREF(var, new);" Other changes: * Replace "old = var; var = new; Py_DECREF(var)" with "Py_SETREF(var, new);" * Replace "old = var; var = new; Py_XDECREF(var)" with "Py_XSETREF(var, new);" * And remove the "old" variable.
I splitted my PR in two commits:
To ease review. Maybe it can be two PRs. |
The Py_XSETREF/Py_SETREF change looks very good; increased readability, which also brings increased maintainability. I'm not quite sure about the Py_CLEAR change. It introduces Py_XDECREF in places where Py_DECREF was previously used1. Footnotes
|
I thought that performance of an if(NULL) was not important, but I can revert this part. |
If you benchmark it, I'm sure the performance hit is lost in noise :) |
@@ -624,8 +624,7 @@ _abc__abc_instancecheck_impl(PyObject *module, PyObject *self, | |||
|
|||
switch (PyObject_IsTrue(result)) { | |||
case -1: | |||
Py_DECREF(result); | |||
result = NULL; | |||
Py_CLEAR(result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_DECREF(var); var=NULL
only has a race condition if var
is accessible from outside the function, right? I don't think there is any race condition if var
is just a local variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for those cases you only get increased readability and fewer lines of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, the code is only unsafe if the variable is shared between two function calls. It can be a static variable inside the function, a global variable, an object attribute, etc. Here result is a local variable, so the code is safe. Do you think that the commit message should be enhanced?
@erlend-aasland is right, it's also about about increasing readability.
I splitted this PR into 3 PRs, but just for Py_SETREF():
I replaced "Fix race condition" with "Fix potential race condition". |