Skip to content
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

Jump to bottom

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

Closed
vstinner opened this issue Nov 16, 2022 · 3 comments
Closed

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

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 •
edited by bedevere-bot

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 vstinner added the type-bug An unexpected behavior, bug, or error label Nov 16, 2022
@bedevere-bot bedevere-bot mentioned this issue Nov 16, 2022
Closed
@vstinner
Copy link
Member Author

vstinner commented Nov 16, 2022

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 vstinner mentioned this issue Nov 16, 2022
Closed
@vstinner
Copy link
Member Author

vstinner commented Nov 16, 2022

See also #98724

This was referenced Nov 21, 2022
Merged
Merged
Merged
@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
@vstinner
gh-99537: Use Py_SETREF() function in longobject C code (#99655)
20d9749 
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
@vstinner
gh-99537: Use Py_SETREF() function in C code (#99657)
135ec7c 
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
@vstinner
gh-99537: Use Py_SETREF() function in C code (#99656)
7e3f09c 
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.
This was referenced Nov 22, 2022
Merged
Merged
vstinner added a commit that referenced this issue Nov 22, 2022
@vstinner
gh-99537: Use Py_CLEAR() function in C code (#99686)
9a91182 
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
@vstinner
gh-99537: Use Py_SETREF(var, NULL) in C code (#99687)
81f7359 
Replace "Py_DECREF(var); var = NULL;" with "Py_SETREF(var, NULL);".
@vstinner
Copy link
Member Author

vstinner commented Nov 23, 2022 •
edited

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@vstinner