functools.partial segfaults in repr when keywords attribute is abused

[MSeifert04]

BPO	29800
Nosy	@ambv, @serhiy-storchaka, @MojoVampire, @MSeifert04
PRs	bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings #649 bpo-29800: [3.6] Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings (#649) #671 bpo-29800: [3.5] Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings (#649) #672 [Do Not Merge] Sample of CPython life with blurb. #703

^{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 2017-03-15.07:44:40.141>
created_at = <Date 2017-03-12.21:10:14.166>
labels = ['3.7', 'library', 'type-crash']
title = 'functools.partial segfaults in repr when keywords attribute is abused'
updated_at = <Date 2017-03-24.22:19:14.562>
user = 'https://github.com/MSeifert04'

bugs.python.org fields:

activity = <Date 2017-03-24.22:19:14.562>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2017-03-15.07:44:40.141>
closer = 'serhiy.storchaka'
components = ['Library (Lib)']
creation = <Date 2017-03-12.21:10:14.166>
creator = 'MSeifert'
dependencies = []
files = []
hgrepos = []
issue_num = 29800
keywords = []
message_count = 11.0
messages = ['289510', '289544', '289546', '289554', '289558', '289566', '289641', '289646', '290180', '290181', '290182']
nosy_count = 4.0
nosy_names = ['lukasz.langa', 'serhiy.storchaka', 'josh.r', 'MSeifert']
pr_nums = ['649', '671', '672', '703']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue29800'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[MSeifert04]

It's possible to create a segfault when one (inappropriatly) changes the functools.partial.keywords attribute manually. A minimal example reproducing the segfault is:

>> from functools import partial
>> p = partial(int)
>> p.keywords[1] = 10
>> repr(p)

System: Windows 10
Python: 3.5.3, 3.6.0, master-branch

[MojoVampire]

I feel like adding a type check to partial_repr might not be the correct fix here. If PyUnicode_FromFormat returned NULL and set an exception here, then the existing code would work as written (raising an exception, but not segfaulting).

Alternatively, if the code in question used %S for the key instead of %U, it would also work as written (not raising an exception or segfaulting).

It's extremely strange to have something accepted, then raise exceptions in the repr of all places, and adding extra special purpose code for that specific purpose seems odd, to say the least.

I think I'd be in favor of using %S personally, since %U should only be used when you have absolute guarantees that the object is a Unicode object, which we can't give here. Sure, an invalid state passes without notice in the repr, but I'm not sure that bothers me so much; if they actually try to call the invalid partial they'll get the TypeError they deserve at a time when the type finally matters.

[serhiy-storchaka]

PyUnicode_FromFormat() crashes in debug build if the argument for %U is not a unicode string.

I thought about using %S. This would correspond possible Python implementation ('%s=%r' % (key, value)). But in that case we should increase refcounts of key and value (and maybe even pto->kw) since custom str of the key can remove the value from the dict and make the value variable the hanging reference.

I would prefer never failing __repr__, but other core developers have other opinion. Currently partial.__repr__ raises an exception on deep recursion and when the repr of any argument raises an exception.

Current Michael's patch LGTM, but if his want to suggest the solution with %S it would LGTM too.

[ambv]

I'd also prefer %S instead of a hard-coded check.

[MSeifert04]

Given that this my first contribution to CPython I'm not too sure about the etiquette. When do I know (or who decides) when an agreement on a fix is reached? I wouldn't mind retracting the pull request if someone else wants to fix it differently.

I actually used the PyUnicode_Check on purpose because I always viewed the representation should be either unreadably (like objectxyz at 0xwhatever) or something that can should be copy&pasted-able. Returning something that will definetly throw an exception when copied seemed inappropriate.

But the %S change definetly has it's attraction: shorter, also fixes the segfault and no need for type checking.

Given Serhiy's answer it seems to me there could be another lurking problem because any %R or %S is potentially a problem within PyDict_Next - because this C-API-Function states that "The dictionary p should not be mutated during iteration. It is safe to modify the values of the keys as you iterate over the dictionary, but only so long as the set of keys does not change." But arbitary __str__ or __repr__ functions could do just that. I'm mostly an end-user so I'm not sure if I understand that right?

[serhiy-storchaka]

It would be nice if you provided the solution with %S. But if you don't do this, and nobody other provide other pull request, I'll merge the current solution.

When the dictionary keys are changed during iteration PyDict_Next() can skip some key-value pairs or return some key-value pairs multiple times. It never raises an exception, crashes or hangs in infinite loop. This is appropriate.

The problem is that after calling __str__ for the key, the borrowed reference to the value can become invalid, and __repr__ will be called for destroyed object. This can cause an undefined behavior, in particular a crash.

[serhiy-storchaka]

Could you create PRs for 3.6 and 3.5 Michael?

[serhiy-storchaka]

Thank you for your contribution Michael!

[serhiy-storchaka]

New changeset 0641ada by Serhiy Storchaka (Michael Seifert) in branch '3.5':
bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings (#649) (#672)
0641ada

[serhiy-storchaka]

New changeset 53b2667 by Serhiy Storchaka (Michael Seifert) in branch '3.6':
bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings (#649) (#671)
53b2667