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

set.update(): Crash when source set is changed during merging #68771

Closed
serhiy-storchaka opened this issue Jul 7, 2015 · 15 comments
Closed

set.update(): Crash when source set is changed during merging #68771

serhiy-storchaka opened this issue Jul 7, 2015 · 15 comments
Assignees
@rhettinger
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@serhiy-storchaka
Copy link
Member

BPO 24583
Nosy @rhettinger, @serhiy-storchaka
Files
  • test_set__merge_and_mutate.patch
  • index_to_entry.diff
  • intermediary.diff
  • set_add_entry_leak.patch
  • set_named_exits.diff
  • set_self_contained.diff: Make the logic self-contained so it can't be called incorrectly.
    		•

    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/rhettinger'
closed_at = <Date 2015-07-20.11:34:55.748>
created_at = <Date 2015-07-07.11:56:10.834>
labels = ['interpreter-core', 'invalid', 'type-crash', 'release-blocker']
title = 'set.update(): Crash when source set is changed during merging'
updated_at = <Date 2015-07-20.11:34:55.747>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2015-07-20.11:34:55.747>
actor = 'rhettinger'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2015-07-20.11:34:55.748>
closer = 'rhettinger'
components = ['Interpreter Core']
creation = <Date 2015-07-07.11:56:10.834>
creator = 'serhiy.storchaka'
dependencies = []
files = ['39881', '39883', '39890', '39952', '39956', '39957']
hgrepos = []
issue_num = 24583
keywords = ['patch']
message_count = 15.0
messages = ['246403', '246471', '246472', '246477', '246513', '246572', '246792', '246947', '246958', '246961', '246962', '246964', '246965', '246970', '246977']
nosy_count = 3.0
nosy_names = ['rhettinger', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'release blocker'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue24583'
versions = ['Python 3.5', 'Python 3.6']
    @serhiy-storchaka
    Copy link
    Member Author

    When the set is not empty and set.update() argument is set that is modified during merging, the crash is caused. Here is a test that reproduces a crash. Only Python 3.5+ is affected.

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Jul 7, 2015
    @rhettinger rhettinger self-assigned this Jul 7, 2015
    @serhiy-storchaka
    Copy link
    Member Author

    LGTM for 3.5.

    But 3.6 has other bug. Changeset 637e197be547 looks incorrect to me. key should be increfed before calling PyObject_RichCompareBool() for the same reason as startkey.

    @rhettinger
    Copy link
    Contributor

    Can you produce a test case?

    Perhaps the incref/decref pair ought to be moved into PyObject_RichCompareBool(). It doesn't make much sense for the callers to do the work.

    @vstinner vstinner changed the title Crash when source set is changed during merging set.update(): Crash when source set is changed during merging Jul 8, 2015
    @serhiy-storchaka
    Copy link
    Member Author

    The same test is crashed in 3.6 even with index_to_entry.diff.

    ./python -m test.regrtest -F -m test_merge_and_mutate test_set

    @serhiy-storchaka
    Copy link
    Member Author

    Perhaps the incref/decref pair ought to be moved into PyObject_RichCompareBool().

    This wouldn't help because key can be used after PyObject_RichCompareBool().

    @serhiy-storchaka
    Copy link
    Member Author

    intermediary.diff LGTM.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 16, 2015

    New changeset 5c3812412b6f by Raymond Hettinger in branch '3.5':
    Issue bpo-24583: Fix crash when set is mutated while being updated.
    https://hg.python.org/cpython/rev/5c3812412b6f

    New changeset 05cb67dab161 by Raymond Hettinger in branch 'default':
    Issue bpo-24583: Fix crash when set is mutated while being updated.
    https://hg.python.org/cpython/rev/05cb67dab161

    @serhiy-storchaka
    Copy link
    Member Author

    5c3812412b6f caused a refleak.

    $ ./python -m test.regrtest -uall -R 3:3 test_set
[1/1] test_set
beginning 6 repetitions
123456
......
test_set leaked [23561, 24961, 23961] references, sum=72483
test_set leaked [785, 787, 787] memory blocks, sum=2359
1 test failed:
    test_set

    Proposed patch fixes this.

    @rhettinger
    Copy link
    Contributor

    3.6 only. Correct?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 20, 2015

    New changeset acb5b177dd4e by Raymond Hettinger in branch 'default':
    Issue bpo-24583: Fix refcount leak.
    https://hg.python.org/cpython/rev/acb5b177dd4e

    @serhiy-storchaka
    Copy link
    Member Author

    AFAIK 3.5+ (not tested).

    @rhettinger
    Copy link
    Contributor

    Added a patch to neaten it up a bit by naming the exit conditions and avoiding the unnecessary extra incref/decref pair around the resize call.

    @rhettinger
    Copy link
    Contributor

    Added a variant patch that brings the steps together in a more logical manner (single entry point at the top and the named exits at the bottom, brings refcount adjustment logic together in a more coherent way). The "restart" target is done the same way as the "top" target in dictobject.c. Added a comment explaining why the pre-increment is necessary.

    @serhiy-storchaka
    Copy link
    Member Author

    Both variants LGTM. But set_self_contained.diff seems better.

    I suppose this is 3.6 only.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 20, 2015

    New changeset 3f2c12c0abdb by Raymond Hettinger in branch 'default':
    Issue bpo-24583: Consolidate previous set object updates into a single function
    https://hg.python.org/cpython/rev/3f2c12c0abdb

    @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

    @rhettinger rhettinger

    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @rhettinger @serhiy-storchaka