Repository navigation
test_*_code functions in _testcapi/getargs.c have memory leaks #110572
Description
Bug report
I don't think it is very important, since this is just a test, but why have it when it is spotted?
test_k_code:cpython/Modules/_testcapi/getargs.c
Lines 331 to 398 in 326c6c4
/* This function not only tests the 'k' getargs code, but also the PyLong_AsUnsignedLongMask() function. */ static PyObject * test_k_code(PyObject *self, PyObject *Py_UNUSED(ignored)) { PyObject *tuple, *num; unsigned long value; tuple = PyTuple_New(1); if (tuple == NULL) { return NULL; } /* a number larger than ULONG_MAX even on 64-bit platforms */ num = PyLong_FromString("FFFFFFFFFFFFFFFFFFFFFFFF", NULL, 16); if (num == NULL) { return NULL; } value = PyLong_AsUnsignedLongMask(num); if (value != ULONG_MAX) { PyErr_SetString(PyExc_AssertionError, "test_k_code: " "PyLong_AsUnsignedLongMask() returned wrong value for long 0xFFF...FFF"); return NULL; } PyTuple_SET_ITEM(tuple, 0, num); value = 0; if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) { return NULL; } if (value != ULONG_MAX) { PyErr_SetString(PyExc_AssertionError, "test_k_code: k code returned wrong value for long 0xFFF...FFF"); return NULL; } Py_DECREF(num); num = PyLong_FromString("-FFFFFFFF000000000000000042", NULL, 16); if (num == NULL) { return NULL; } value = PyLong_AsUnsignedLongMask(num); if (value != (unsigned long)-0x42) { PyErr_SetString(PyExc_AssertionError, "test_k_code: " "PyLong_AsUnsignedLongMask() returned wrong value for long -0xFFF..000042"); return NULL; } PyTuple_SET_ITEM(tuple, 0, num); value = 0; if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) { return NULL; } if (value != (unsigned long)-0x42) { PyErr_SetString(PyExc_AssertionError, "test_k_code: k code returned wrong value for long -0xFFF..000042"); return NULL; } Py_DECREF(tuple); Py_RETURN_NONE; }
On errors tuple is not decrefed.
Also, note these lines:
cpython/Modules/_testcapi/getargs.c
Lines 358 to 374 in 326c6c4
| PyTuple_SET_ITEM(tuple, 0, num); | |
| value = 0; | |
| if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) { | |
| return NULL; | |
| } | |
| if (value != ULONG_MAX) { | |
| PyErr_SetString(PyExc_AssertionError, | |
| "test_k_code: k code returned wrong value for long 0xFFF...FFF"); | |
| return NULL; | |
| } | |
| Py_DECREF(num); | |
| num = PyLong_FromString("-FFFFFFFF000000000000000042", NULL, 16); | |
| if (num == NULL) { | |
| return NULL; | |
| } |
Here' we leave a tuple is a semi-broken state. Its 0'th item has a reference count of 0.
We should also recreate a tuple here with the new items. test_L_code also has this problem.
cpython/Modules/_testcapi/getargs.c
Lines 684 to 732 in 326c6c4
static PyObject * test_L_code(PyObject *self, PyObject *Py_UNUSED(ignored)) { PyObject *tuple, *num; long long value; tuple = PyTuple_New(1); if (tuple == NULL) { return NULL; } num = PyLong_FromLong(42); if (num == NULL) { return NULL; } PyTuple_SET_ITEM(tuple, 0, num); value = -1; if (!PyArg_ParseTuple(tuple, "L:test_L_code", &value)) { return NULL; } if (value != 42) { PyErr_SetString(PyExc_AssertionError, "test_L_code: L code returned wrong value for long 42"); return NULL; } Py_DECREF(num); num = PyLong_FromLong(42); if (num == NULL) { return NULL; } PyTuple_SET_ITEM(tuple, 0, num); value = -1; if (!PyArg_ParseTuple(tuple, "L:test_L_code", &value)) { return NULL; } if (value != 42) { PyErr_SetString(PyExc_AssertionError, "test_L_code: L code returned wrong value for int 42"); return NULL; } Py_DECREF(tuple); Py_RETURN_NONE; }
On errors tuple is not decrefed. And num is re-assigned without tuple cleanup.
cpython/Modules/_testcapi/getargs.c
Lines 734 to 767 in 326c6c4
/* Test the s and z codes for PyArg_ParseTuple. */ static PyObject * test_s_code(PyObject *self, PyObject *Py_UNUSED(ignored)) { /* Unicode strings should be accepted */ PyObject *tuple = PyTuple_New(1); if (tuple == NULL) { return NULL; } PyObject *obj = PyUnicode_Decode("t\xeate", strlen("t\xeate"), "latin-1", NULL); if (obj == NULL) { return NULL; } PyTuple_SET_ITEM(tuple, 0, obj); /* These two blocks used to raise a TypeError: * "argument must be string without null bytes, not str" */ char *value; if (!PyArg_ParseTuple(tuple, "s:test_s_code1", &value)) { return NULL; } if (!PyArg_ParseTuple(tuple, "z:test_s_code2", &value)) { return NULL; } Py_DECREF(tuple); Py_RETURN_NONE; }
As well, tuple is leaked on errors.
I have a PR ready.
Activity
pythongh-110572: Fix leaks in `test_*_code` in `_testcapi/getargs.c`
serhiy-storchaka commentedon Oct 11, 2023
Are these tests still needed?
sobolevn commentedon Oct 11, 2023
's','L', and'k'are still mentioned in https://docs.python.org/3.12/howto/clinic.html#how-to-use-real-argument-clinic-converters-instead-of-legacy-convertersSo, I guess we still need to make sure that they are supported and work correctly.
They are even not deprecated, just called "legacy".
serhiy-storchaka commentedon Oct 11, 2023
I mean that they are tested in
Lib/test/test_capi/test_getargs.pyusing wrappers likegetargs_k(). And #110628 adds tests for functions likePyLong_AsUnsignedLongMask().sobolevn commentedon Oct 11, 2023
In this case, yes. We can move some logic from these tests to
test_long.py.Things that right now are not tested in
test_long(this list might be not full):test_k_code:PyLong_FromString("FFFFFFFFFFFFFFFFFFFFFFFF", NULL, 16)test_k_code:PyLong_FromString("-FFFFFFFF000000000000000042", NULL, 16)test_s_code:getargs_s('t\xeate')(however,'abc\xe9'is tested)test_s_code:getargs_z('t\xeate')(however,'abc\xe9'is tested)So, do you agree on this plan?
test_*_codeare in eithertest_long.pyor intest_getargs.pytest_long.pyand possible PRs totest_getargs.py1.with these tests removedgh-110572: Fix potential leaks in test_*_code in _testcapi/getargs.c (G…
pythongh-110572: Fix potential leaks in test_*_code in _testcapi/geta…
serhiy-storchaka commentedon Oct 21, 2023
I merged your PR, and it will be backported to 3.12. In main we should consider removing this tests.
PyLong_FromString("FFFFFFFFFFFFFFFFFFFFFFFF", NULL, 16)is just a way to create a long int for the following test ofPyLong_AsUnsignedLongMask(). There are already tests forPyLong_AsUnsignedLongMask()for argument outside of 0..ULONG_MAX. There are also similar tests for the "k" format unit. I think that all this can be deleted.[3.12] gh-110572: Fix potential leaks in test_*_code in _testcapi/get…
pythongh-110572: Remove `test_*` from `_testcapi/getargs.c`
gh-110572: Remove `test_*` from `_testcapi/getargs.c` (GH-111214)
hugovk commentedon Nov 9, 2023
Thanks for the PRs.
pythongh-110572: Fix potential leaks in test_*_code in _testcapi/geta…
pythongh-110572: Remove `test_*` from `_testcapi/getargs.c` (pythonGH…
pythongh-110572: Fix potential leaks in test_*_code in _testcapi/geta…
pythongh-110572: Remove `test_*` from `_testcapi/getargs.c` (pythonGH…