Reference counting bug in PyArg_ParseTuple and PyArg_ParseTupleAndKeywords

@loewis

BPO	6083
Nosy	@loewis, @birkenfeld, @gpshead, @abalkin, @taleinat, @serhiy-storchaka, @imz, @ananthan-123
Dependencies	bpo-20191: resource.prlimit(int, int, str) crashs
Files	python-bug-01.patch: Patch to fix the problem test-resource.py: Test for Modules/resource.c test-ctypes.py: Test for Modules/_ctypes/_ctypes.c test-functools.py: Test for Modules/_functoolsmodule.c (py3k only) issue6083.diff PyArg_ParseTuple_refcount.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 = None
closed_at = None
created_at = <Date 2009-05-22.08:18:54.577>
labels = ['interpreter-core', 'type-crash']
title = 'Reference counting bug in PyArg_ParseTuple and PyArg_ParseTupleAndKeywords'
updated_at = <Date 2020-02-20.07:51:55.428>
user = 'https://bugs.python.org/billm'

bugs.python.org fields:

activity = <Date 2020-02-20.07:51:55.428>
actor = 'taleinat'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2009-05-22.08:18:54.577>
creator = 'billm'
dependencies = ['20191']
files = ['14040', '19616', '19617', '19618', '20400', '27567']
hgrepos = []
issue_num = 6083
keywords = ['patch']
message_count = 25.0
messages = ['88181', '88204', '88215', '121285', '126226', '126234', '172912', '181297', '181316', '181327', '181592', '181600', '182108', '182110', '201454', '314264', '314268', '314273', '314285', '314286', '314287', '314289', '322381', '362299', '362302']
nosy_count = 11.0
nosy_names = ['loewis', 'georg.brandl', 'gregory.p.smith', 'belopolsky', 'taleinat', 'billm', 'abacabadabacaba', 'python-dev', 'serhiy.storchaka', 'imz', 'Ananthakrishnan']
pr_nums = []
priority = 'high'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue6083'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

Linked PRs

gh-50333: Deprecate support of non-tuple sequences in PyArg_ParseTuple() #128374

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.

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?

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.

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).

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.

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.

Here is a patch which get rid of all three PyArg_ParseTuple usage with parsing nested sequences. Thanks Evgeny for reproducers.

And will the next call be effective (do anything), if we have already set the limit with the testing call?

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.

Ivan, can you supply a PR or would you like someone else to do so?

I want to do a PR,if this is still needeed.

Please do, Ananthakrishnan!

No longer relevant because resource.setrlimit is now ported to Argument Clinic.

While the issue was fixed for resource.setrlimit and others (before it was ported to Argument Clinic), the root issue has not yet been fixed.

pythongh-50333: Deprecate support of non-tuple sequences in PyArg_Par…

#128374 implements a more lenient variant of the initial deprecation plan. Non-tuple sequences are only deprecated if the nested format units store borrowed buffer or reference (e.g. "s" and "O"). If "(items)" only contains format units like "i" or "d" (or "s*"), general sequences are still accepted.

gh-50333: Deprecate support of non-tuple sequences in PyArg_ParseTupl…

Navigation Menu

Repository navigation

Reference counting bug in PyArg_ParseTuple and PyArg_ParseTupleAndKeywords #50333

Linked PRs

18 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Reference counting bug in PyArg_ParseTuple and PyArg_ParseTupleAndKeywords #50333

Description

Linked PRs

Activity

billm commented on May 22, 2009

birkenfeld commented on May 22, 2009

loewis commented on May 22, 2009

abacabadabacaba commented on Nov 16, 2010

abalkin commented on Jan 14, 2011

abalkin commented on Jan 14, 2011

serhiy-storchaka commented on Oct 14, 2012

18 remaining items

imz commented on Mar 22, 2018

serhiy-storchaka commented on Mar 22, 2018

taleinat commented on Jul 25, 2018

ananthan-123 commented on Feb 20, 2020

taleinat commented on Feb 20, 2020

arhadthedev commented on Apr 29, 2023

serhiy-storchaka commented on Dec 31, 2024

serhiy-storchaka commented on Dec 31, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions