Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

functools.partial segfaults in repr when keywords attribute is abused #73986

Closed
MSeifert04 mannequin opened this issue Mar 12, 2017 · 11 comments
Closed

functools.partial segfaults in repr when keywords attribute is abused #73986

MSeifert04 mannequin opened this issue Mar 12, 2017 · 11 comments
Assignees
Labels
3.7 (EOL) end of life stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@MSeifert04
Copy link
Mannequin

MSeifert04 mannequin commented Mar 12, 2017

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
    Copy link
    Mannequin Author

    MSeifert04 mannequin commented Mar 12, 2017

    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

    @MSeifert04 MSeifert04 mannequin added 3.7 (EOL) end of life stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump labels Mar 12, 2017
    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Mar 13, 2017

    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
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka self-assigned this Mar 13, 2017
    @ambv
    Copy link
    Contributor

    ambv commented Mar 13, 2017

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

    @MSeifert04
    Copy link
    Mannequin Author

    MSeifert04 mannequin commented Mar 14, 2017

    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
    Copy link
    Member

    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
    Copy link
    Member

    Could you create PRs for 3.6 and 3.5 Michael?

    @serhiy-storchaka
    Copy link
    Member

    Thank you for your contribution Michael!

    @serhiy-storchaka
    Copy link
    Member

    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
    Copy link
    Member

    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

    @serhiy-storchaka
    Copy link
    Member

    New changeset 6c3d527 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)
    6c3d527

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants