| msg88181 - (view) |
Author: Bill McCloskey (billm) |
Date: 2009-05-22 08:18 |
| The code for resource_setrlimit in Modules/resource.c does not handle reference counting properly. The following Python code segfaults for me on Ubuntu 8.10 in Python 2.5.2 and also a custom-built 2.6.1. -- import resource l = [0, 0] class MyNum: def __int__(self): l[1] = 20 return 10 def __del__(self): print 'byebye', self l[0] = MyNum() l[1] = MyNum() resource.setrlimit(resource.RLIMIT_CPU, l) -- The problem is that setrlimit gets its arguments by calling: PyArg_ParseTuple(args, "i(OO):setrlimit", &resource, &curobj, &maxobj) The references curobj and maxobj are borrowed. The second argument can be passed as a mutable list rather than a tuple, so it's possible to update the list in the middle of setrlimit, causing maxobj to be destroyed before setrlimit is done with it. I've attached a patch that INCREFs both variables immediately after parsing them to avoid this problem. In my opinion it seems dangerous to allow format strings with the 'O' specifier appearing in parentheses. You normally expect that objects returned from PyArg_ParseTuple are pretty safe, but the fact that the inner sequence may be mutable violates this assumption. Might it make sense to ban this use case? I only found one other instance of it in the Python source tree, inside ctypes. This one may also be a crashing bug--I didn't look at it carefully enough. |
|
|
| msg88204 - (view) |
Author: Georg Brandl (georg.brandl) *  |
Date: 2009-05-22 17:09 |
| That is a good point. IMHO we'll be fine with a warning in the docs, and fixing our own two instances. Martin, what do you think? |
|
|
| msg88215 - (view) |
Author: Martin v. Löwis (loewis) * |
Date: 2009-05-22 20:14 |
| IMO, any refcounting bug has the potential as a security risk. So I think we should deprecate this with a warning, and eventually remove it, as billm proposes. It's probably debatable whether to backport the warning to 2.6 or earlier; I think we shouldn't, as many applications are probably valid. |
|
|
| msg121285 - (view) |
Author: Evgeny Kapun (abacabadabacaba) |
Date: 2010-11-16 12:33 |
| Actually, this can't be fixed without modifying C API methods PyArg_ParseTuple and PyArg_ParseTupleAndKeywords, because it's possible to make an object deallocated before PyArg_ParseTuple returns, so Py_INCREF immediately after parsing would be already too late. Here are my test cases: test-resource.py - in Modules/resource.c, and python-bug-01.patch won't work against it. test-ctypes.py - in Modules/_ctypes/_ctypes.c. test-functools.py - in Modules/_functoolsmodule.c (py3k only). |
|
|
| msg126226 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2011-01-14 04:55 |
| Let me summarize the issue: the PyArg_ParseTuple format code 'O' returns a borrowed reference. However, when the 'O' code appears inside parenthesis, there may not be an object to hold the reference to borrow from. This is what happens in the test-functools.py crasher: partial.__setstate__() takes a 4-tuple argument that is unpacked using a "(OOOO)" format. The test case passes an instance instead of a tuple that supports the sequence methods, but does not hold the reference to the "items" that its []-operator returns. This is not a problem at the top level because args argument to PyArg_ParseTuple is always a real tuple. I think that rather than deprecating the use of 'O' format inside parentheses, "(..O..)" unpacking should reject to unpack arguments other than tuples or maybe lists. |
|
|
| msg126234 - (view) |
Author: Alexander Belopolsky (belopolsky) * |
Date: 2011-01-14 06:42 |
| Attached patch passes the regrtest and makes test-functools.py raise an exception rather than crash. The proposed change will make functions like partial.__setstate__ require tuple argument even though currently it would accept any container. This is not an issue with __setstate__ because it should only be called with arguments produced by __reduce__ and in the case of partial, __reduce__ produces state as a tuple. Other functions may need to be modified if they need to continue to accept arbitrary sequences. |
|
|
| msg172912 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2012-10-14 20:18 |
| Here is a patch which get rid of all three PyArg_ParseTuple usage with parsing nested sequences. Thanks Evgeny for reproducers. |
|
|
| msg181297 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2013-02-03 20:31 |
| Serhiy's patch looks good to me. |
|
|
| msg181316 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-02-04 11:05 |
| New changeset a4c85f9b8f58 by Serhiy Storchaka in branch '2.7': Issue #6083: Fix multiple segmentation faults occured when PyArg_ParseTuple http://hg.python.org/cpython/rev/a4c85f9b8f58 New changeset 4bac47eb444c by Serhiy Storchaka in branch '3.2': Issue #6083: Fix multiple segmentation faults occured when PyArg_ParseTuple http://hg.python.org/cpython/rev/4bac47eb444c New changeset e0ee10f27e5f by Serhiy Storchaka in branch '3.3': Issue #6083: Fix multiple segmentation faults occured when PyArg_ParseTuple http://hg.python.org/cpython/rev/e0ee10f27e5f New changeset 3e3a7d825736 by Serhiy Storchaka in branch 'default': Issue #6083: Fix multiple segmentation faults occured when PyArg_ParseTuple http://hg.python.org/cpython/rev/3e3a7d825736 |
|
|
| msg181327 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-02-04 13:33 |
| I do not have possibility and desires blind-repair a test on alien platform, so just temporarily disable a new test in Lib/ctypes/test/test_returnfuncptrs.py on Windows. If someone has a desire to fix it fell free to do this. I do not close this issue because committed patch only fix existing crashes in Python. There should be plenty of such bugs in third-party code. We have to deprecate this unsafe feature or reject any sequences except tuple as Alexander proposed. |
|
|
| msg181592 - (view) |
Author: Stefan Krah (skrah) * |
Date: 2013-02-07 11:41 |
| The FreeBSD 6.4 bot is failing, too. Note that the other functions in test_returnfuncptrs.py do this in order to get strchr(): dll = CDLL(_ctypes_test.__file__) get_strchr = dll.get_strchr get_strchr.restype = CFUNCTYPE(c_char_p, c_char_p, c_char) strchr = get_strchr() |
|
|
| msg181600 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-02-07 13:05 |
| There are 6 different ways to get a function (see comment around PyCFuncPtr_new() in Modules/_ctypes/_ctypes.c). The other tests just use other ways. I'm more carefully read ctype code and found my mistake. Need to import "my_strchr", and not "strchr". |
|
|
| msg182108 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-02-14 16:11 |
| FreeBSD 6.4 and Windows test failures was fixed in changesets 8fb98fb758e8 and ec70abe8c886. |
|
|
| msg182110 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-02-14 16:16 |
| Oh, I shouldn't close this until this dangerous feature will be deprecated. |
|
|
| msg201454 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-10-27 12:49 |
| Accepting an arbitrary sequence when "(...)" is used in the format string was introduced in changeset 0ef1071cb7fe. |
|
|
| msg314264 - (view) |
Author: Ivan Zakharyaschev (imz) |
Date: 2018-03-22 14:54 |
| > New changeset a4c85f9b8f58 by Serhiy Storchaka in branch '2.7': Issue #6083: Fix multiple segmentation faults occured when PyArg_ParseTuple http://hg.python.org/cpython/rev/a4c85f9b8f58 This test has a problem: though it tests not the ability to set a CPU hard limit, it fails if the hard limit is limited. Perhaps, ignore any exception there? Could you please help me re-write it correctly, so that I can run it on gyle--ALT's builder host--successfully): # Issue 6083: Reference counting bug def test_setrusage_refcount(self): try: limits = resource.getrlimit(resource.RLIMIT_CPU) except AttributeError: self.skipTest('RLIMIT_CPU not available') class BadSequence: def __len__(self): return 2 def __getitem__(self, key): if key in (0, 1): return len(tuple(range(1000000))) raise IndexError resource.setrlimit(resource.RLIMIT_CPU, BadSequence()) The failure: [builder@team ~]$ python /usr/lib64/python2.7/test/test_resource.py test_args (__main__.ResourceTest) ... ok test_fsize_enforced (__main__.ResourceTest) ... ok test_fsize_ismax (__main__.ResourceTest) ... ok test_fsize_toobig (__main__.ResourceTest) ... ok test_getrusage (__main__.ResourceTest) ... ok test_setrusage_refcount (__main__.ResourceTest) ... ERROR ====================================================================== ERROR: test_setrusage_refcount (__main__.ResourceTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/lib64/python2.7/test/test_resource.py", line 117, in test_setrusage_refcount resource.setrlimit(resource.RLIMIT_CPU, BadSequence()) ValueError: not allowed to raise maximum limit ---------------------------------------------------------------------- Ran 6 tests in 0.085s FAILED (errors=1) Traceback (most recent call last): File "/usr/lib64/python2.7/test/test_resource.py", line 123, in test_main() File "/usr/lib64/python2.7/test/test_resource.py", line 120, in test_main test_support.run_unittest(ResourceTest) File "/usr/lib64/python2.7/test/support/__init__.py", line 1577, in run_unittest _run_suite(suite) File "/usr/lib64/python2.7/test/support/__init__.py", line 1542, in _run_suite raise TestFailed(err) test.support.TestFailed: Traceback (most recent call last): File "/usr/lib64/python2.7/test/test_resource.py", line 117, in test_setrusage_refcount resource.setrlimit(resource.RLIMIT_CPU, BadSequence()) ValueError: not allowed to raise maximum limit [builder@team ~]$ |
|
|
| msg314268 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-03-22 15:59 |
| What does resource.getrlimit(resource.RLIMIT_CPU) return? |
|
|
| msg314273 - (view) |
Author: Ivan Zakharyaschev (imz) |
Date: 2018-03-22 16:59 |
| >>> import resource >>> resource.getrlimit(resource.RLIMIT_CPU) (7200, 7260) |
|
|
| msg314285 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-03-22 18:39 |
| The simplest way is to try passing the limit as a tuple resource.setrlimit(resource.RLIMIT_CPU, (1000000, 1000000)) and skip the test if it failed. |
|
|
| msg314286 - (view) |
Author: Ivan Zakharyaschev (imz) |
Date: 2018-03-22 18:47 |
| Thanks! I also thought about this simplest way. What about this: diff --git a/Python/Lib/test/test_resource.py b/Python/Lib/test/test_resource.py index de29d3b..bec4440 100644 --- a/Python/Lib/test/test_resource.py +++ b/Python/Lib/test/test_resource.py @@ -102,16 +102,21 @@ class ResourceTest(unittest.TestCase): # Issue 6083: Reference counting bug def test_setrusage_refcount(self): + howmany = 1000000 try: limits = resource.getrlimit(resource.RLIMIT_CPU) except AttributeError: self.skipTest('RLIMIT_CPU not available') + try: + resource.setrlimit(resource.RLIMIT_CPU, (howmany, howmany)) + except _: + self.skipTest('Setting RLIMIT_CPU not possible') class BadSequence: def __len__(self): return 2 def __getitem__(self, key): if key in (0, 1): - return len(tuple(range(1000000))) + return len(tuple(range(howmany))) raise IndexError resource.setrlimit(resource.RLIMIT_CPU, BadSequence()) What should I write instead of _? |
|
|
| msg314287 - (view) |
Author: Ivan Zakharyaschev (imz) |
Date: 2018-03-22 18:48 |
| And will the next call be effective (do anything), if we have already set the limit with the testing call? |
|
|
| msg314289 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2018-03-22 20:23 |
| LGTM. > What should I write instead of _? (ValueError, OSError) > And will the next call be effective (do anything), if we have already set the limit with the testing call? This doesn't matter. We test that it doesn't crash when parse arguments. |
|
|
| msg322381 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2018-07-25 18:16 |
| Ivan, can you supply a PR or would you like someone else to do so? |
|
|
| msg362299 - (view) |
Author: Ananthakrishnan (Ananthakrishnan) * |
Date: 2020-02-20 07:20 |
| I want to do a PR,if this is still needeed. |
|
|
| msg362302 - (view) |
Author: Tal Einat (taleinat) * |
Date: 2020-02-20 07:51 |
| Please do, Ananthakrishnan! |
|
|