This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: functools.partial segfaults in repr when keywords attribute is abused
Type: crash Stage: resolved
Components: Library (Lib) Versions: Python 3.7, Python 3.6, Python 3.5
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: MSeifert, josh.r, lukasz.langa, serhiy.storchaka
Priority: normal Keywords:

Created on 2017-03-12 21:10 by MSeifert, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 649 merged MSeifert, 2017-03-12 21:13
PR 671 merged MSeifert, 2017-03-15 07:03
PR 672 merged MSeifert, 2017-03-15 07:09
PR 703 larry, 2017-03-17 21:00
Messages (11)
msg289510 - (view) Author: Michael Seifert (MSeifert) * Date: 2017-03-12 21:10
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
msg289544 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2017-03-13 17:51
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.
msg289546 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-13 18:46
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.
msg289554 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2017-03-13 22:25
I'd also prefer %S instead of a hard-coded check.
msg289558 - (view) Author: Michael Seifert (MSeifert) * Date: 2017-03-14 01:53
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?
msg289566 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-14 05:55
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.
msg289641 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-15 06:22
Could you create PRs for 3.6 and 3.5 Michael?
msg289646 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-15 07:44
Thank you for your contribution Michael!
msg290180 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 22:18
New changeset 0641ada9b78b6a06f539d4bde42c1ad1b90579a7 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)
msg290181 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 22:18
New changeset 53b2667dcf2a7d13af466a5fb91844f5125a920d 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)
msg290182 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-03-24 22:19
New changeset 6c3d5274687c97f9c13800ad50e73e15b54f629d by Serhiy Storchaka (Michael Seifert) in branch 'master':
bpo-29800: Fix crashes in partial.__repr__ if the keys of partial.keywords are not strings (#649)
Date User Action Args
2022-04-11 14:58:44adminsetgithub: 73986
2017-03-24 22:19:14serhiy.storchakasetmessages: + msg290182
2017-03-24 22:18:58serhiy.storchakasetmessages: + msg290181
2017-03-24 22:18:49serhiy.storchakasetmessages: + msg290180
2017-03-17 21:00:36larrysetpull_requests: + pull_request623
2017-03-15 07:44:40serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg289646

stage: needs patch -> resolved
2017-03-15 07:09:36MSeifertsetpull_requests: + pull_request555
2017-03-15 07:03:30MSeifertsetpull_requests: + pull_request554
2017-03-15 06:22:36serhiy.storchakasetmessages: + msg289641
2017-03-14 05:55:40serhiy.storchakasetmessages: + msg289566
stage: needs patch
2017-03-14 01:53:20MSeifertsetmessages: + msg289558
2017-03-13 22:25:04lukasz.langasetnosy: + lukasz.langa
messages: + msg289554
2017-03-13 18:46:35serhiy.storchakasetassignee: serhiy.storchaka
2017-03-13 18:46:13serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg289546
2017-03-13 17:51:19josh.rsetnosy: + josh.r
messages: + msg289544
2017-03-12 21:13:07MSeifertsetpull_requests: + pull_request536
2017-03-12 21:10:14MSeifertcreate