msg258708 - (view) |
Author: (squidevil) |
Date: 2016-01-20 18:26 |
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.
|
msg259204 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-01-29 11:08 |
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.
|
msg259222 - (view) |
Author: (squidevil) |
Date: 2016-01-29 21:40 |
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?
|
msg259224 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-01-29 23:57 |
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.
|
msg259226 - (view) |
Author: (squidevil) |
Date: 2016-01-30 00:22 |
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.
|
msg259242 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-01-30 07:39 |
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.
|
msg259252 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-01-30 11:23 |
Previous attempt to fix reference leaks was in issue984722.
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 issue20024 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.
|
msg259254 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-01-30 11:36 |
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.
|
msg259841 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-02-08 13:05 |
Following patch is written in the style of current code for tuples.
|
msg265504 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-05-14 04:27 |
Patch 3 looks good to me
|
msg265515 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-05-14 10:51 |
Thank you Martin.
|
msg265612 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-05-15 12:52 |
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.
|
msg265658 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-05-16 02:06 |
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.
|
msg265679 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-05-16 07:58 |
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.
|
msg265684 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-05-16 08:19 |
Yeah okay, I agree that the code was already saving the references in a container, so it makes sense to keep doing that.
|
msg265954 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-05-20 19:32 |
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 #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 #26168: Fixed possible refleaks in failing Py_BuildValue() with the "N"
https://hg.python.org/cpython/rev/03b32f854680
|
msg265955 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-05-20 19:33 |
Thank you for your review Martin.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:26 | admin | set | github: 70356 |
2016-05-20 19:33:34 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages:
+ msg265955
stage: commit review -> resolved |
2016-05-20 19:32:26 | python-dev | set | nosy:
+ python-dev messages:
+ msg265954
|
2016-05-16 08:19:20 | martin.panter | set | messages:
+ msg265684 |
2016-05-16 07:58:03 | serhiy.storchaka | set | messages:
+ msg265679 |
2016-05-16 02:06:24 | martin.panter | set | messages:
+ msg265658 |
2016-05-15 12:52:00 | serhiy.storchaka | set | files:
+ pybuildvalue_leak4.patch
messages:
+ msg265612 |
2016-05-14 10:51:04 | serhiy.storchaka | set | messages:
+ msg265515 stage: patch review -> commit review |
2016-05-14 04:27:29 | martin.panter | set | messages:
+ msg265504 |
2016-02-08 13:05:29 | serhiy.storchaka | set | files:
+ pybuildvalue_leak3.patch
messages:
+ msg259841 |
2016-01-30 11:36:59 | martin.panter | set | messages:
+ msg259254 |
2016-01-30 11:23:39 | serhiy.storchaka | set | files:
+ pybuildvalue_leak2.patch nosy:
+ mwh, rhettinger, shredwheat messages:
+ msg259252
|
2016-01-30 07:39:25 | serhiy.storchaka | set | type: resource usage -> crash messages:
+ msg259242 |
2016-01-30 00:22:59 | squidevil | set | messages:
+ msg259226 |
2016-01-29 23:57:13 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg259224
|
2016-01-29 21:40:30 | squidevil | set | messages:
+ msg259222 |
2016-01-29 11:08:51 | serhiy.storchaka | set | files:
+ pybuildvalue_leak.patch messages:
+ msg259204
assignee: serhiy.storchaka keywords:
+ patch stage: needs patch -> patch review |
2016-01-20 19:33:40 | josh.r | set | nosy:
+ josh.r
|
2016-01-20 18:27:52 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka stage: needs patch
versions:
+ Python 3.5, Python 3.6 |
2016-01-20 18:26:25 | squidevil | create | |