[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[Kyle Stanley]

Kyle Stanley  added the comment:

Thanks for the PR, Zackery.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[miss-islington]

miss-islington  added the comment:


New changeset 7b78e7f9fd77bb3280ee39fb74b86772a7d46a70 by Zackery Spytz in 
branch 'master':
bpo-40061: Fix a possible refleak in _asynciomodule.c (GH-19748)
https://github.com/python/cpython/commit/7b78e7f9fd77bb3280ee39fb74b86772a7d46a70


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[Kyle Stanley]

Kyle Stanley  added the comment:

Antoine Pitrou wrote:
> You're right that a Py_DECREF is missing there.  However, it seems unlikely 
> that this is triggering the test failure, because `PyList_New(1)` will 
> practically never fail in normal conditions (and even making it deliberately 
> fail would be quite difficult).

Thanks for clarifying; spotting refleaks is something that I've only recently 
started to get the hang of, so I wanted to double check first. :-)

I should have updated this issue, but I resolved the main test_asyncio failure 
that I saw from GH-19149 in GH-19282. Either way though, I think this could 
still be fixed (even if it would rarely be an issue in most situations).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[Antoine Pitrou]

Antoine Pitrou  added the comment:

You're right that a Py_DECREF is missing there.  However, it seems unlikely 
that this is triggering the test failure, because `PyList_New(1)` will 
practically never fail in normal conditions (and even making it deliberately 
fail would be quite difficult).

--
nosy: +pitrou

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[Zackery Spytz]

Change by Zackery Spytz :


--
keywords: +patch
nosy: +ZackerySpytz
nosy_count: 3.0 -> 4.0
pull_requests: +19069
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/19748

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[Kyle Stanley]

Kyle Stanley  added the comment:

> When using the `test-with-buildbots` label in GH-19149 (which involved no C 
> changes), a failure occurred in test_asyncio for several of the refleak 
> buildbots.

To clarify, the refleak failures occurred when i applied the label to the 
following commit: 
https://github.com/python/cpython/pull/19149/commits/21a12e9340644c81e758ddf20fc9034f265d1930

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue40061] Possible refleak in _asynciomodule.c future_add_done_callback()

[Kyle Stanley]

New submission from Kyle Stanley :

When using the `test-with-buildbots` label in GH-19149 (which involved no C 
changes), a failure occurred in test_asyncio for several of the refleak 
buildbots. Here's the output of a few:

AMD64 Fedora Stable Refleaks PR:

test_asyncio leaked [3, 3, 27] references, sum=33
test_asyncio leaked [3, 3, 28] memory blocks, sum=34
2 tests failed again:
test__xxsubinterpreters test_asyncio
== Tests result: FAILURE then FAILURE ==

AMD64 RHEL8 Refleaks PR:

test_asyncio leaked [3, 3, 3] references, sum=9
test_asyncio leaked [3, 3, 3] memory blocks, sum=9
2 tests failed again:
test__xxsubinterpreters test_asyncio
== Tests result: FAILURE then FAILURE ==

RHEL7 Refleaks PR:

test_asyncio leaked [3, 3, 3] references, sum=9
test_asyncio leaked [3, 3, 3] memory blocks, sum=9
2 tests failed again:
test__xxsubinterpreters test_asyncio
== Tests result: FAILURE then FAILURE ==


I'm unable to replicate it locally, but I think I may have located a subtle, 
uncommon refleak in `future_add_done_callback()`, within _asynciomodule.c. 
Specifically:

```
PyObject *tup = PyTuple_New(2);
if (tup == NULL) {
return NULL;
}
Py_INCREF(arg);
PyTuple_SET_ITEM(tup, 0, arg);
Py_INCREF(ctx);
PyTuple_SET_ITEM(tup, 1, (PyObject *)ctx);

if (fut->fut_callbacks != NULL) {
int err = PyList_Append(fut->fut_callbacks, tup);
if (err) {
Py_DECREF(tup);
return NULL;
}
Py_DECREF(tup);
}
else {
fut->fut_callbacks = PyList_New(1);
if (fut->fut_callbacks == NULL) {
// Missing ``Py_DECREF(tup);`` ?
return NULL;
}
```

(The above code is located at: 
https://github.com/python/cpython/blob/7668a8bc93c2bd573716d1bea0f52ea520502b28/Modules/_asynciomodule.c#L664-L685)

In the above conditional for "if (fut->fut_callbacks == NULL)", it appears that 
`tup` is pointing to a non-NULL new reference at this point, and thus should be 
decref'd prior to returning NULL. Otherwise, it seems like it could be leaked.

But, I would appreciate it if someone could double check this (the C-API isn't 
an area I'm experienced); particularly since this code has been in place for a 
decent while (since 3.7). I _suspect_ it's gone undetected and only failed 
intermittently because this specific ``return NULL`` path is rather uncommon.

I'd be glad to open a PR to address the issue, assuming I'm not missing 
something with the above refleak. Otherwise, feel free to correct me.

--
assignee: aeros
components: C API, Extension Modules
messages: 364985
nosy: aeros, asvetlov, yselivanov
priority: high
severity: normal
status: open
title: Possible refleak in _asynciomodule.c future_add_done_callback()
type: resource usage
versions: Python 3.7, Python 3.8, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com