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