Crash when modifying the **kwargs passed to a function.

[amauryfa]

BPO	2016
Nosy	@gvanrossum, @loewis, @terryjreedy, @gpshead, @amauryfa, @abalkin, @devdanzin, @bitdancer
Files	fix2016.txt: use PyString_CheckExact for keyword names. issue2016.diff: Patch against revision 62083

^{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: ```python assignee = 'https://github.com/amauryfa' closed_at = created_at = labels = ['interpreter-core', 'type-crash'] title = 'Crash when modifying the **kwargs passed to a function.' updated_at = user = 'https://github.com/amauryfa' ``` bugs.python.org fields: ```python activity = actor = 'belopolsky' assignee = 'amaury.forgeotdarc' closed = True closed_date = closer = 'amaury.forgeotdarc' components = ['Interpreter Core'] creation = creator = 'amaury.forgeotdarc' dependencies = [] files = ['9411', '9912'] hgrepos = [] issue_num = 2016 keywords = ['patch'] message_count = 15.0 messages = ['62086', '62104', '62105', '62107', '62109', '62290', '62325', '64785', '86588', '89716', '104548', '104560', '116943', '116945', '116946'] nosy_count = 9.0 nosy_names = ['gvanrossum', 'loewis', 'terry.reedy', 'gregory.p.smith', 'amaury.forgeotdarc', 'belopolsky', 'ajaksu2', 'r.david.murray', 'BreamoreBoy'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = 'commit review' status = 'closed' superseder = None type = 'crash' url = 'https://bugs.python.org/issue2016' versions = ['Python 2.7'] ```

[amauryfa]

The following script exploits a comment in funcobject.c: / XXX This is broken if the caller deletes dict items! \/ Because the code only borrows references to the items, it is possible to have them destroyed before they are copied into the called frame.

class Name(str):
  def __eq__(self, other):
     del x[self]
     return str.__eq__(self, other)
  def __hash__(self):
     return str.__hash__(self)

x = {Name("a"):1, Name("b"):2}
def f(a, b): print a,b

f(**x)   # Segfault

[abalkin]

It also appears that the tuple and dict checks in function_call are redundant. If there is a use case that results in function_call getting anything (non null) other than dict and tuple in arg and kw, I don't think silently ignoring such arguments is the right behavior.

[amauryfa]

This could be turned into an assertion. But beware that some extension writers may use code like PyObject_Call(callable, args, Py_None) which works perfectly today.

[abalkin]

According to http://docs.python.org/api/object.html,

"""
PyObject* PyObject_Call(    PyObject *callable_object, PyObject *args,
PyObject *kw)
    Return value: New reference.
    Call a callable Python object callable_object, with arguments given
by the tuple args, and named arguments given by the dictionary kw. If no
named arguments are needed, kw may be NULL. args must not be NULL, use
an empty tuple if no arguments are needed. Returns the result of the
call on success, or NULL on failure. 
"""

passing Py_None as kw is not allowed. Interestingly, the documentation also says that "args must not be NULL," while the code is clearly more forgiving.

[abalkin]

Sorry, please ignore my last comment about arg. The null and tuple checks are for argdefs, not arg.

[gvanrossum]

I have a proposed minimal fix, but I wonder if this would break existing code (apart from the exploit given here). Martin? (I think the discussion between Alexander and Amaury can be ignored for the purpose of reviewing the fix; only the exploit matters.)

[61337411-43fc-4a9c-b8d5-4060aede66d0]

I think we agree that this patch has the potential of breaking existing valid code. So based on the policy that we should avoid doing so in a bugfix release, I'd rather reject that fix (fix2016.txt) for 2.5.x. OTOH, if it is really unlikely that is ever occurs in existing code, there would be no point in backporting it to 2.5.x, since the check wouldn't trigger.

I also can't see a security concern - applications shouldn't pass untrusted objects as keyword arguments (if they were, such objects could put their malicious code inside __hash__).

[abalkin]

I am attaching my fix along the lines of a solution suggested by Amaury at http://mail.python.org/pipermail/python-dev/2008- February/076747.html: """

Or is the proper fix to incref the values going into the kw array and decref them upon exit?

Yet Another Kind Of Tuple... However this seems the correct thing to do. """

I did not do any performance tests with this patch.

[90baf024-6604-450d-8341-d796fe6858f3]

Confirmed in py3k (debug), trunk (non-debug) hangs with lots of CPU activity.

py3k backtrace:

0 0x0805c196 in do_richcompare (v=0x83e734c, w=0xb7afa648, op=2) at

Objects/object.c:561

1 0x0805c450 in PyObject_RichCompare (v=0x83e734c, w=0xb7afa648, op=2)

at Objects/object.c:611

2 0x0805c4f8 in PyObject_RichCompareBool (v=0x83e734c, w=0xb7afa648,

op=2) at Objects/object.c:633

3 0x080b4140 in PyEval_EvalCodeEx (co=0x836b748, globals=0xb7d4e8f4,

locals=0x0, args=0xb7d14048, argcount=0, kws=0xb7ac1c08, kwcount=2, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0) at Python/ceval.c:3019

4 0x08168f6c in function_call (func=0x839fa84, arg=0xb7d14034,

kw=0x82998f4) at Objects/funcobject.c:628

5 0x08136e66 in PyObject_Call (func=0x839fa84, arg=0xb7d14034,

kw=0x82998f4) at Objects/abstract.c:2161

6 0x080b74d5 in ext_do_call (func=0x839fa84, pp_stack=0xbfe58674,

flags=2, na=0, nk=0) at Python/ceval.c:4045

7 0x080b1ae7 in PyEval_EvalFrameEx (f=0x828b57c, throwflag=0) at

Python/ceval.c:2571

8 0x080b49fe in PyEval_EvalCodeEx (co=0x8370da8, globals=0xb7d4e8f4,

locals=0xb7d4e8f4, args=0x0, argcount=0, kws=0x0, kwcount=0, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0) at Python/ceval.c:3180

9 0x080a901d in PyEval_EvalCode (co=0x8370da8, globals=0xb7d4e8f4,

locals=0xb7d4e8f4) at Python/ceval.c:650

10 0x080e269c in run_mod (mod=0x84112f0, filename=0x81bef10 "\<stdin>",

globals=0xb7d4e8f4, locals=0xb7d4e8f4, flags=0xbfe58ab8, arena=0x8348338) at Python/pythonrun.c:1697

11 0x080e08d7 in PyRun_InteractiveOneFlags (fp=0xb7e9e440,

filename=0x81bef10 "\<stdin>", flags=0xbfe58ab8) at Python/pythonrun.c:1091

12 0x080e03dc in PyRun_InteractiveLoopFlags (fp=0xb7e9e440,

filename=0x81bef10 "\<stdin>", flags=0xbfe58ab8) at Python/pythonrun.c:993

13 0x080e0234 in PyRun_AnyFileExFlags (fp=0xb7e9e440,

filename=0x81bef10 "\<stdin>", closeit=0, flags=0xbfe58ab8) at Python/pythonrun.c:962

14 0x080f7fc4 in Py_Main (argc=1, argv=0xb7d12028) at Modules/main.c:625

15 0x0805b22e in main (argc=1, argv=0xbfe59be4) at ./Modules/python.c:71

trunk backtrace: Program received signal SIGINT, Interrupt. [Switching to Thread 0xb7da68c0 (LWP 22330)] 0x080ff6a8 in PyTraceBack_Print (v=0xb7bf1e3c, f=0xb7d780c0) at Python/traceback.c:243 243 while (tb1 != NULL) { (gdb) bt

0 0x080ff6a8 in PyTraceBack_Print (v=0xb7bf1e3c, f=0xb7d780c0) at

Python/traceback.c:243

1 0x080f6f56 in PyErr_Display (exception=0x816a540, value=0xb7bf504c,

tb=0xb7bf1e3c) at Python/pythonrun.c:1200

2 0x080fd7ed in sys_excepthook (self=0x0, args=0xb7bf1d4c) at

Python/sysmodule.c:138

3 0x0805e8e5 in PyObject_Call (func=0xb7d7728c, arg=0xb7bf1d4c,

kw=0x0) at Objects/abstract.c:2506

4 0x080cf882 in PyEval_CallObjectWithKeywords (func=0xb7d7728c,

arg=0xb7bf1d4c, kw=0x0) at Python/ceval.c:3781

5 0x080f78ce in PyErr_PrintEx (set_sys_last_vars=1) at

Python/pythonrun.c:1146

6 0x080f7f9c in PyRun_InteractiveOneFlags (fp=0xb7ef2440,

filename=0x8144910 "\<stdin>", flags=0xbfe8a348) at Python/pythonrun.c:1037

7 0x080f80f6 in PyRun_InteractiveLoopFlags (fp=0xb7ef2440,

filename=0x8144910 "\<stdin>", flags=0xbfe8a348) at Python/pythonrun.c:763

8 0x080f89e2 in PyRun_AnyFileExFlags (fp=0xb7ef2440,

filename=0x8144910 "\<stdin>", closeit=0, flags=0xbfe8a348) at Python/pythonrun.c:732

9 0x0805ac4e in Py_Main (argc=0, argv=0xbfe8a414) at Modules/main.c:599

10 0x08059eb2 in main (argc=Cannot access memory at address 0x1

) at ./Modules/python.c:23

[amauryfa]

Fixed in trunk with r73564.

I performed performance tests: differences with pybench were negligible (\<1%), but a specially crafted case like: kw = dict(a=1, b=2, c=3) for x in xrange(self.rounds): f(**kw) showed an improvement of 21%!

Will backport to various branches after 3.1 is out.

[terryjreedy]

Is everything fixed, so this can be closed?

[bitdancer]

It was merged to py3k in r73623, 3.1 in r73625, but not, as far as I can see, to 2.6.

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

Can someone please backport this to 2.7 so we can get this closed, thanks.

[amauryfa]

The fix was applied to trunk before the creation of the 2.7 branch. There is nothing to backport

[abalkin]

On Mon, Sep 20, 2010 at 10:23 AM, Mark Lawrence \report@bugs.python.org\ wrote:

Mark Lawrence \breamoreboy@yahoo.co.uk\ added the comment:

Can someone please backport this to 2.7 so we can get this closed, thanks.

AFAICT, r73564 preceded 2.7 branch cut, so the fix is already in 2.7.