Skip to content

Use Py_SETREF() macro to fix race conditions (potential race conditions) in C extensions #99537

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

Closed
vstinner opened this issue Nov 16, 2022 · 3 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

vstinner commented Nov 16, 2022

A lot of C code in Python uses this pattern:

Py_XDECREF(var);
var = value;

If var was holding the last strong reference to a Python object, the object finalizer is called which can call arbitrary Python code which can access the var variable which is now a dangling pointer, and so Python does just crash in this case.

The correct pattern is:

old_var = var;
var = value;
Py_XDECREF(old_var);

And Py_SETREF() can be used to make this code shorter and easier to read and maintain:

Py_XSETREF(var, value);

While the pattern is unsafe, maybe it's not possible to crash Python. But I prefer to replace this pattern with Py_SETREF() or Py_XSETREF() so I don't have to think about: oh, is it possible to crash Python?

Again, Py_SETREF() also makes the code shorter, easier to read and maintain.

While we are here, I also propose to replace:

Py_XDECREF(var);
var = NULL;

with:

Py_CLEAR(var);

which is shorter than:

Py_XSETREF(var, NULL);

See also issue #99300 "Replace Py_INCREF()/Py_XINCREF() usage with Py_NewRef()/Py_XNewRef()" and issue #99481 "Add Py_HOLD_REF() macro to hold a strong reference to a Python object while executing code".

Linked PRs

@vstinner
Copy link
Member Author

I wrote an implementation of this idea: PR #99538.

@vstinner vstinner changed the title Use Py_SETREF() macro to fix race conditions (poentital crashes) in C extensions Use Py_SETREF() macro to fix race conditions (potential crashes) in C extensions Nov 16, 2022
@vstinner
Copy link
Member Author

See also #98724

@vstinner vstinner changed the title Use Py_SETREF() macro to fix race conditions (potential crashes) in C extensions Use Py_SETREF() macro to fix race conditions (potential race conditions) in C extensions Nov 21, 2022
vstinner added a commit that referenced this issue Nov 22, 2022
Replace "Py_DECREF(var); var = new;" with "Py_SETREF(var, new);"
in longobject.c and _testcapi/long.c.
vstinner added a commit that referenced this issue Nov 22, 2022
Fix potential race condition in 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.
vstinner added a commit that referenced this issue Nov 22, 2022
Fix potential race condition in 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.
vstinner added a commit that referenced this issue Nov 22, 2022
Replace "Py_XDECREF(var); var = NULL;" with "Py_CLEAR(var);".

Don't replace "Py_DECREF(var); var = NULL;" with "Py_CLEAR(var);". It
would add an useless "if (var)" test in code path where var cannot be
NULL.
vstinner added a commit that referenced this issue Nov 23, 2022
Replace "Py_DECREF(var); var = NULL;" with "Py_SETREF(var, NULL);".
@vstinner
Copy link
Member Author

vstinner commented Nov 23, 2022

Ok, I converted all patterns to Py_SETREF(), Py_XSETREF() or Py_CLEAR(). I close the issue. Thanks for reviews!

Quick comparison with git grep -E 'Py_X?SETREF'|wc -l:

  • Python 3.11: 294 lines
  • Python 3.12: 458 lines (1.6x more)

iakov added a commit to iakov/pythonqt that referenced this issue Feb 8, 2023
More details on [XSETREF macro for safer code](python/cpython#99537)
iakov added a commit to iakov/pythonqt that referenced this issue Feb 8, 2023
More details on [XSETREF macro for safer code](python/cpython#99537)
mrbean-bremen pushed a commit to MeVisLab/pythonqt that referenced this issue Feb 9, 2023
More details on [XSETREF macro for safer code](python/cpython#99537)
jcfr pushed a commit to jcfr/PythonQt that referenced this issue Dec 14, 2023
More details on [XSETREF macro for safer code](python/cpython#99537)

(cherry picked and adapted from commit MeVisLab/pythonqt@MeVisLab/pythonqt@17ea22a)
jcfr pushed a commit to commontk/PythonQt that referenced this issue Jan 28, 2024
More details on [XSETREF macro for safer code](python/cpython#99537)

(cherry picked and adapted from commit MeVisLab/pythonqt@MeVisLab/pythonqt@17ea22a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

1 participant