[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: Thank you for your review Martin. -- resolution: -> fixed stage: commit review -> resolved status: open -> closed ___ Python tracker

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Roundup Robot added the comment: New changeset 4e605f809551 by Serhiy Storchaka in branch '3.5': Issue #26168: Fixed possible refleaks in failing Py_BuildValue() with the "N" https://hg.python.org/cpython/rev/4e605f809551 New changeset 0285173d81b4 by Serhiy Storchaka in branch '2.7': Issue

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Martin Panter added the comment: Yeah okay, I agree that the code was already saving the references in a container, so it makes sense to keep doing that. -- ___ Python tracker

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: Currently do_mktuple(), do_mklist() and do_mkdict() save references in a collection. I think that there are reasons to do this, and third-party code can be broken if just deallocate references. pybuildvalue_leak4.patch just implements this strategy more

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Martin Panter added the comment: Is it really a problem if the old value is deallocated? It sounds like a similar case to , and would only be a problem if you passed a borrowed reference, and relied on the reference staying alive for another

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: I'm not happy with pybuildvalue_leak3.patch. For failed keys it saves values with the same key (None). This means that old value can be deallocated before the end of building all dict. Following patch collects all values after error in a tuple. This not

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: Thank you Martin. -- stage: patch review -> commit review ___ Python tracker ___

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Martin Panter added the comment: Patch 3 looks good to me -- ___ Python tracker ___ ___ Python-bugs-list

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: Following patch is written in the style of current code for tuples. -- Added file: http://bugs.python.org/file41850/pybuildvalue_leak3.patch ___ Python tracker

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Martin Panter added the comment: I was going to suggest just documenting your use-after-free case as a limitation of the N format. I doubt there is much demand to give away a borrowed reference with N _and_ rely on that borrowed reference staying alive for other parameters. It seems a lot of

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: Current code already decref obj if creating previous argument is failed. But a leak if PyTuple_New() fails is possible. Proposed patch fixes this leak and adds a test for the case when creating previous argument is failed. Testing the failure of

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

squidevil added the comment: Martin Panter: You're right. The DECREF on v when itemfailed will decref the N object and prevent the leak. My original analysis was wrong on that count. You're right, do_mklist and do_mkdict (in 2.7.11 at least) have similar problems, bailing after list or dict

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Serhiy Storchaka added the comment: Rather rare reference leak is not the worst bug here. Following example const char *s = ...; PyObject *b = PyBytes_New(...); return PyBuildValue("(Ns)s", b, s, PyBytes_AS_STRING(b)); works if s is correct UTF-8 encoded string. But if it is not

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

squidevil added the comment: It looks like this patch is against the "default" cpython (3.x) branch. The 2.7 branch is missing the if(itemfailed) code in do_mktuple whose else clause was modified by the patch. It looks like some of this needs to be backported to 2.7? --

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Changes by Serhiy Storchaka : -- nosy: +serhiy.storchaka stage: -> needs patch versions: +Python 3.5, Python 3.6 ___ Python tracker

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

New submission from squidevil: Expected behavior: Calling Py_BuildValue with a 'N' argument should take ownership of the N object, and on failure (returns NULL), call Py_DECREF() on any N argument. The documentation explicitly says that this is the intended usage: "N": Same as "O",

[issue26168] Py_BuildValue may leak 'N' arguments on PyTuple_New failure

Changes by Josh Rosenberg : -- nosy: +josh.r ___ Python tracker ___ ___