Py_BuildValue may leak 'N' arguments on PyTuple_New failure

[squidevil]

BPO	26168
Nosy	@mwhudson, @rhettinger, @vadmium, @serhiy-storchaka, @MojoVampire
Files	pybuildvalue_leak.patch pybuildvalue_leak2.patch pybuildvalue_leak3.patch pybuildvalue_leak4.patch

^{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/serhiy-storchaka'
closed_at = <Date 2016-05-20.19:33:34.401>
created_at = <Date 2016-01-20.18:26:25.437>
labels = ['interpreter-core', 'type-crash']
title = "Py_BuildValue may leak 'N' arguments on PyTuple_New failure"
updated_at = <Date 2016-05-20.19:33:34.400>
user = 'https://bugs.python.org/squidevil'

bugs.python.org fields:

activity = <Date 2016-05-20.19:33:34.400>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-05-20.19:33:34.401>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-01-20.18:26:25.437>
creator = 'squidevil'
dependencies = []
files = ['41751', '41757', '41850', '42858']
hgrepos = []
issue_num = 26168
keywords = ['patch']
message_count = 17.0
messages = ['258708', '259204', '259222', '259224', '259226', '259242', '259252', '259254', '259841', '265504', '265515', '265612', '265658', '265679', '265684', '265954', '265955']
nosy_count = 8.0
nosy_names = ['mwh', 'rhettinger', 'shredwheat', 'python-dev', 'martin.panter', 'serhiy.storchaka', 'josh.r', 'squidevil']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue26168'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[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", except it doesn't increment the reference count on the object. Useful when the object is created by a call to an object constructor in the argument list.

Actual behavior:
N objects appear to be abandoned/leaked in some cases.

Example: PyBuildValue("iN", 0, obj);

• calls _Py_BuildValue_SizeT via macro
• calls va_build_value (in modsupport.c)
• calls do_mktuple [0]
• [0] first calls v = PyTuple_New(n=2). If this fails, it returns NULL, leaking obj.
• if [0] creates the tuple v, then it goes on to populate the values in the tuple.
• [0] calls do_mkvalue() to create the "i=0" object. If this fails, obj is never Py_DECREF()'ed.

Many other leaks are possible, as long as at least one allocation occurs prior to the processing of the N arguments.

[serhiy-storchaka]

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 PyTuple_New() is problematic.

[squidevil]

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?

[vadmium]

Left a review.

Squidevil: You say if do_mkvalue() fails, the N object is not released. But looking at the code, I think it gets stored in the tuple, and then the tuple is released: <https://hg.python.org/cpython/file/v3.5.1/Python/modsupport.c#l182\>. So the only leak I can see is if the tuple construction fails.

Serhiy’s patch fixes do_mktuple(), but I think the same problem would affect do_mkdict() and do_mklist() as well.

[squidevil]

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 creation failure, without continuing to process the rest of the items.

[serhiy-storchaka]

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 correct UTF-8 encoded string, the first "s" is failed and the inner tuple is deallocated together with the borrowed reference to b. The second "s" then reads freed memory.

[serhiy-storchaka]

Previous attempt to fix reference leaks was in bpo-984722.

Proposed patch uses different approach. It correctly fixes reference leaks after error in processing dict items (like "{()s(())N"), fixes leaksafter memory errors on creating tuple, list or dict, fixes bpo-20024 for lists and dicts, fixes use-after-free issue described in msg259242.

But the patch has a disadvantage in comparison with current code. If user converter ("O&") accepts Python objects and steal its reference (as "N"), this reference will be leaked. This is a price for avoiding potential use-after-free issues. Current stdlib code doesn't use converters that steal Python object reference. May be we should document this limitation.

[vadmium]

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 churn just to avoid a theoretical problem.

[serhiy-storchaka]

Following patch is written in the style of current code for tuples.

[vadmium]

Patch 3 looks good to me

[serhiy-storchaka]

Thank you Martin.

[serhiy-storchaka]

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 only fixes the issue with building dict, but makes the code for building tuple, list and dict cleaner. It no longer contains the code for processing after error, it is moved in separate function.

[vadmium]

Is it really a problem if the old value is deallocated? It sounds like a similar case to <https://bugs.python.org/issue26168#msg259242\>, and would only be a problem if you passed a borrowed reference, and relied on the reference staying alive for another argument.

I do like the separate do_ignore() function in patch 4, but I don’t think it is worthwhile allocating a temporary tuple to save values in. The allocation can fail. Also, I understand do_mktuple() etc are recursive, so nested borrowed references would still be released before the outer do_ignore() function releases the outer tuple.

[serhiy-storchaka]

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 reliably. It guaranties (unless tuple allocation fails) that borrowed references in the same container or upper levels are kept until all content of the container is proceeded.

The more perfect solution is to allocate one list on error, save all references to it and deallocate it at the end of PyBuildValue(). But this leads to more complicated code.

[vadmium]

Yeah okay, I agree that the code was already saving the references in a container, so it makes sense to keep doing that.

[python-dev]

New changeset 4e605f809551 by Serhiy Storchaka in branch '3.5':
Issue bpo-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 bpo-26168: Fixed possible refleaks in failing Py_BuildValue() with the "N"
https://hg.python.org/cpython/rev/0285173d81b4

New changeset 03b32f854680 by Serhiy Storchaka in branch 'default':
Issue bpo-26168: Fixed possible refleaks in failing Py_BuildValue() with the "N"
https://hg.python.org/cpython/rev/03b32f854680

[serhiy-storchaka]

Thank you for your review Martin.