Possible refleak in _asynciomodule.c future_add_done_callback()

[aeros]

BPO	40061
Nosy	@pitrou, @asvetlov, @1st1, @ZackerySpytz, @miss-islington, @aeros
PRs	bpo-40061: Fix a possible refleak in _asynciomodule.c #19748

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/aeros'
closed_at = <Date 2020-05-30.08:25:36.071>
created_at = <Date 2020-03-25.09:36:53.444>
labels = ['3.8', '3.9', 'performance', 'extension-modules', 'expert-C-API', '3.7']
title = 'Possible refleak in _asynciomodule.c future_add_done_callback()'
updated_at = <Date 2020-05-30.08:25:36.070>
user = 'https://github.com/aeros'

bugs.python.org fields:

activity = <Date 2020-05-30.08:25:36.070>
actor = 'aeros'
assignee = 'aeros'
closed = True
closed_date = <Date 2020-05-30.08:25:36.071>
closer = 'aeros'
components = ['Extension Modules', 'C API']
creation = <Date 2020-03-25.09:36:53.444>
creator = 'aeros'
dependencies = []
files = []
hgrepos = []
issue_num = 40061
keywords = ['patch']
message_count = 6.0
messages = ['364985', '365029', '367507', '367511', '370365', '370366']
nosy_count = 6.0
nosy_names = ['pitrou', 'asvetlov', 'yselivanov', 'ZackerySpytz', 'miss-islington', 'aeros']
pr_nums = ['19748']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue40061'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[aeros]

When using the test-with-buildbots label in #63348 (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:

cpython/Modules/_asynciomodule.c

Lines 664 to 685 in 7668a8b

	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) {
	return NULL;
	}

)

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.