Repository navigation
gh-145376: Fix reference leaks in _collectionsmodule.c - #145421
vstinner merged 1 commit into
Conversation
Sorry, something went wrong.
| iternext = *Py_TYPE(it)->tp_iternext; | ||
| while ((item = iternext(it)) != NULL) { | ||
| if (deque_appendleft_lock_held(deque, item, maxlen) == -1) { | ||
| Py_DECREF(item); |
There was a problem hiding this comment.
iternext returns a new reference. Should we leave this line unchanged?
Sorry, something went wrong.
There was a problem hiding this comment.
I agree, this is fishy. Does deque_append[left]_lock_held steal item, or not?
Sorry, something went wrong.
There was a problem hiding this comment.
With this change, deque_appendleft_lock_held() always steal its item argument. So removing Py_DECREF(item) here is correct.
Sorry, something went wrong.
There was a problem hiding this comment.
The deque_appendleft_lock_held indeed steals the second argument reference. I wrote this in the OP, but did not add a comment in the code.
If you agree, I will add a comment to the start of deque_append_lock_held/deque_appendleft_lock_held.
Sorry, something went wrong.
There was a problem hiding this comment.
I see now, thanks!
Maybe it is worth to add a comment, that deque_appendleft_lock_held steals reference to the second argument?
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
| iternext = *Py_TYPE(it)->tp_iternext; | ||
| while ((item = iternext(it)) != NULL) { | ||
| if (deque_appendleft_lock_held(deque, item, maxlen) == -1) { | ||
| Py_DECREF(item); |
There was a problem hiding this comment.
With this change, deque_appendleft_lock_held() always steal its item argument. So removing Py_DECREF(item) here is correct.
Sorry, something went wrong.
|
Merged. Thanks for the fix! |
Sorry, something went wrong.
Fix a reference leak if newblock() fails in _collections.deque.
Make deque_append_lock_held and deque_appendleft_lock_held consume the second argument (the methods already consumed the second argument, but not in the error path).
Issues found using Claude.