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.

Sign up for GitHub

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

Jump to bottom

collections.OrderedDict constructor (odict_new) doesn't handle PyDict_New() failure #69180

Closed
vstinner opened this issue Sep 3, 2015 · 15 comments
Closed

collections.OrderedDict constructor (odict_new) doesn't handle PyDict_New() failure #69180

vstinner opened this issue Sep 3, 2015 · 15 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@vstinner
Copy link
Member

vstinner commented Sep 3, 2015

BPO 24992
Nosy @rhettinger, @vstinner, @larryhastings, @ericsnowcurrently, @serhiy-storchaka, @1st1
Files
  • odict.patch
  • odict_failmalloc.py
  • odict-2.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 = <Date 2015-09-03.15:51:57.531>
created_at = <Date 2015-09-03.09:53:53.272>
labels = ['type-bug']
title = "collections.OrderedDict constructor (odict_new) doesn't handle PyDict_New() failure"
updated_at = <Date 2015-09-04.00:10:33.705>
user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2015-09-04.00:10:33.705>
actor = 'eric.snow'
assignee = 'none'
closed = True
closed_date = <Date 2015-09-03.15:51:57.531>
closer = 'vstinner'
components = []
creation = <Date 2015-09-03.09:53:53.272>
creator = 'vstinner'
dependencies = []
files = ['40334', '40335', '40340']
hgrepos = []
issue_num = 24992
keywords = ['patch']
message_count = 15.0
messages = ['249625', '249626', '249639', '249640', '249645', '249648', '249649', '249651', '249652', '249656', '249663', '249665', '249675', '249682', '249702']
nosy_count = 7.0
nosy_names = ['rhettinger', 'vstinner', 'larry', 'python-dev', 'eric.snow', 'serhiy.storchaka', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24992'
versions = ['Python 3.5', 'Python 3.6']
    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    If PyDict_New() fails (ex: memory allocation failure), odict_new() returns a new OrderedDict with an exception set. It's a bug. Attached patch fixes it.

    odict_new() constructor also returns NULL without destroying the newly created object if _odict_initialize() fails. My patch also fixes this.

    My patch inlines _odict_initialize() into odict_new() and avoids useless initialization to 0.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    You can try the odict_failmalloc.py program with a Python compiled in debug mode to see the bug.

    The script requires:
    https://pypi.python.org/pypi/pyfailmalloc

    @serhiy-storchaka
    Copy link
    Member

    If don't initialize fields, then they will be not initialized in odict_dealloc, odict_tp_clear and odict_traverse. But _odict_FIRST(od) and od->od_weakreflist are used in these functions.

    I would allocate a dict for od_inst_dict before calling PyDict_Type.tp_new. An allocator can release GIL and call Python code, and at that moment the OrderedDict object is in inconsistent state. I already were fell in similar trap with lru_cache (bpo-14373, msg246573).

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    If don't initialize fields, then they will be not initialized in odict_dealloc

    Old code initialized all fields to zero (or NULL), like "_odict_FIRST(od) = NULL;". The type allocator fills the newly allocated with zeros. So setting fields again to zero is redundant (useless).

    I would allocate a dict for od_inst_dict before calling PyDict_Type.tp_new. An allocator can release GIL and call Python code, and at that moment the OrderedDict object is in inconsistent state.

    Yes, but the newly created object is not still private at this point, there is only one reference known in the C code. dict_new() has the same design. You can please elaborate the issue?

    We only give the reference to the caller when the newly created OrderedDict is fully initialized (consistent).

    @serhiy-storchaka
    Copy link
    Member

    We only give the reference to the caller when the newly created OrderedDict
    is fully initialized (consistent).

    See msg246573. PyDict_New() can trigger garbage collecting and traversing ,
    and GC have a reference to underinitialized OrderedDict and can call
    odict_traverse() for it.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    See msg246573. PyDict_New() can trigger garbage collecting and traversing and GC have a reference to underinitialized OrderedDict and can call odict_traverse() for it.

    Oooh ok, I understand.

    I updated my patch to implement your idea.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    @serhiy: Python 3.5 is impacted. Do you consider this bug serious enough to request a pull request in Larry's branch for Python 3.5.0?

    @serhiy-storchaka
    Copy link
    Member

    It looks to me as an ordinal bug and that is encountered only in special circumstances with small probability. I think it can wait for 3.5.1.

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    It looks to me as an ordinal bug and that is encountered only in special circumstances with small probability. I think it can wait for 3.5.1.

    Ok, I agree. What about the second patch, does it look ok?

    @serhiy-storchaka
    Copy link
    Member

    I left a nitpick. In any case the patch LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 3, 2015

    New changeset ef1f5aebe1a6 by Victor Stinner in branch '3.5':
    Issue bpo-24992: Fix error handling and a race condition (related to garbage
    https://hg.python.org/cpython/rev/ef1f5aebe1a6

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    I left a nitpick. In any case the patch LGTM.

    Ok, fixed.

    I pushed my fix. Thanks for the review Serhiy.

    @vstinner vstinner closed this as completed Sep 3, 2015
    @1st1
    Copy link
    Member

    1st1 commented Sep 3, 2015

    Is this something that we should ship in 3.5.0rc3?

    @vstinner
    Copy link
    Member Author

    vstinner commented Sep 3, 2015

    Yury wrote:

    Is this something that we should ship in 3.5.0rc3?

    I don't think so. I agree with Serhiy who wrote:

    It looks to me as an ordinal bug and that is encountered only in special circumstances with small probability. I think it can wait for 3.5.1.

    It looks like the bug can only occurs in case of very low memory (an empty dict takes 1 KB or less) which is a rare use case.

    @ericsnowcurrently
    Copy link
    Member

    Thanks for taking care of this, Victor (and Serhiy). :)

    @ericsnowcurrently ericsnowcurrently added the type-bug An unexpected behavior, bug, or error label Sep 4, 2015
    @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
    Assignees
    No one assigned
    Labels
    type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @vstinner @1st1 @ericsnowcurrently @serhiy-storchaka