gh-145376: Fix reference leaks in _collectionsmodule.c#145421
gh-145376: Fix reference leaks in _collectionsmodule.c#145421eendebakpt wants to merge 1 commit intopython:mainfrom
Conversation
| 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?
There was a problem hiding this comment.
I agree, this is fishy. Does deque_append[left]_lock_held steal item, or not?
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.
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.
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?
| 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.
Make
deque_append_lock_heldanddeque_appendleft_lock_heldconsume the second argument (the methods already consumed the second argument, but not in the error path).Issues found using Claude.