gh-115243: Fix crash in deque.index() when the deque is concurrently modified #115247
Conversation
Eclips4
added
skip news
needs backport to 3.11
Co-authored-by: Kirill Podoprigora <[email protected]>
Co-authored-by: Radislav Chugunov <[email protected]>
|
Please add a NEWS entry. It is a user visible change. |
| @@ -0,0 +1 @@ | |||
| Fix possible crashes when operating with the functions in the :func:`deque.index`and custom comparison operators. | |||
There was a problem hiding this comment.
| Fix possible crashes when operating with the functions in the :func:`deque.index`and custom comparison operators. | |
| Fix possible crashes in :func:`deque.index` when calling | |
| :c:func:`PyObject_RichCompareBool`. | |
There was a problem hiding this comment.
I suggest to revert this change. The original wording was more correct.
It has nothing with PyObject_RichCompareBool (it is not the only C API for comparison, and you can trigger a crash from Python code).
There was a problem hiding this comment.
In Python, deque.index invokes PyObject_RichCompareBool in C code. Subsequently, PyObject_RichCompareBool calls the object's __eq__ method in Python. However, I am unsure which explanation is clearer. If the goal is to eliminate the mention of PyObject_RichCompareBool because deque.index always calls it, then the suggestion you provided would indeed be clearer
There was a problem hiding this comment.
It is an implementation detail. The common Python user does not call PyObject_RichCompareBool(), and can be affected by the bug.
How this bug affects Python users?
If deque.index() is called and simultaneously (in other thread, in garbage collector) the same deque is modified, the result is an undefined behavior, possibly crash. You only need to use deque.index() and modify the deque simultaneously. Please describe the effect of this change in terms of the visible behavior change in Python code.
There was a problem hiding this comment.
Serhiy is right. Also, I remember same wording for the same change: 2d5bf56
There was a problem hiding this comment.
If this change refers to my patched code, I believe it will be safe. This is because the issue of use-after-free arises within the inner code of PyObject_RichCompareBool, specifically after the object's reference count has been incremented
Co-authored-by: Kirill Podoprigora <[email protected]>
Misc/NEWS.d/next/Security/2024-02-12-00-33-01.gh-issue-115243.e1oGX8.rst
Outdated
Show resolved
Hide resolved
serhiy-storchaka
changed the title
|
"Custom comparison operator" is just the simplest way to reproduce the issue for tests. |
|
Thanks @kcatss for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12. |
…ently modified (pythonGH-115247) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
…ently modified (pythonGH-115247) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
|
GH-115465 is a backport of this pull request to the 3.12 branch. |
|
GH-115466 is a backport of this pull request to the 3.11 branch. |
…rently modified (GH-115247) (GH-115465) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
…rently modified (GH-115247) (GH-115466) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
…concurrently modified (pythonGH-115247) (pythonGH-115466) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
…concurrently modified (pythonGH-115247) (pythonGH-115466) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
…concurrently modified (pythonGH-115247) (pythonGH-115466) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
…concurrently modified (pythonGH-115247) (pythonGH-115466) (cherry picked from commit 6713601) Co-authored-by: kcatss <[email protected]>
Increase the reference count of item to prevent it from being freed