classification
Title: set.update(): Crash when source set is changed during merging
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: python-dev, rhettinger, serhiy.storchaka
Priority: release blocker Keywords: patch

Created on 2015-07-07 11:56 by serhiy.storchaka, last changed 2015-07-20 11:34 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
test_set__merge_and_mutate.patch serhiy.storchaka, 2015-07-07 11:56 review
index_to_entry.diff rhettinger, 2015-07-08 15:54 review
intermediary.diff rhettinger, 2015-07-10 03:24 review
set_add_entry_leak.patch serhiy.storchaka, 2015-07-19 19:20 review
set_named_exits.diff rhettinger, 2015-07-20 05:34 review
set_self_contained.diff rhettinger, 2015-07-20 05:55 Make the logic self-contained so it can't be called incorrectly. review
Messages (15)
msg246403 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-07 11:56
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.
msg246471 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-08 20:13
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.
msg246472 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-07-08 21:40
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.
msg246477 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-09 02:09
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
msg246513 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-09 17:39
> Perhaps the incref/decref pair ought to be moved into PyObject_RichCompareBool().

This wouldn't help because key can be used after PyObject_RichCompareBool().
msg246572 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-10 17:43
intermediary.diff LGTM.
msg246792 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-16 06:54
New changeset 5c3812412b6f by Raymond Hettinger in branch '3.5':
Issue #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 #24583: Fix crash when set is mutated while being updated.
https://hg.python.org/cpython/rev/05cb67dab161
msg246947 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-19 19:20
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.
msg246958 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-07-20 05:12
3.6 only.  Correct?
msg246961 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-20 05:23
New changeset acb5b177dd4e by Raymond Hettinger in branch 'default':
Issue #24583: Fix refcount leak.
https://hg.python.org/cpython/rev/acb5b177dd4e
msg246962 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-20 05:24
AFAIK 3.5+ (not tested).
msg246964 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-07-20 05:34
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.
msg246965 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-07-20 05:55
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.
msg246970 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-07-20 08:23
Both variants LGTM. But set_self_contained.diff seems better.

I suppose this is 3.6 only.
msg246977 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-20 11:34
New changeset 3f2c12c0abdb by Raymond Hettinger in branch 'default':
Issue #24583:  Consolidate previous set object updates into a single function
https://hg.python.org/cpython/rev/3f2c12c0abdb
History
Date User Action Args
2015-07-20 11:34:55rhettingersetstatus: open -> closed
resolution: not a bug
stage: patch review -> resolved
2015-07-20 11:34:13python-devsetmessages: + msg246977
2015-07-20 08:23:50serhiy.storchakasetmessages: + msg246970
2015-07-20 05:55:57rhettingersetfiles: + set_self_contained.diff

messages: + msg246965
2015-07-20 05:34:26rhettingersetfiles: + set_named_exits.diff

messages: + msg246964
2015-07-20 05:24:26serhiy.storchakasetmessages: + msg246962
2015-07-20 05:23:40python-devsetmessages: + msg246961
2015-07-20 05:12:59rhettingersetmessages: + msg246958
2015-07-19 19:20:59serhiy.storchakasetstatus: closed -> open
files: + set_add_entry_leak.patch
messages: + msg246947

resolution: fixed -> (no value)
priority: high -> release blocker
2015-07-16 06:58:35rhettingersetstatus: open -> closed
resolution: fixed
2015-07-16 06:54:21python-devsetnosy: + python-dev
messages: + msg246792
2015-07-10 17:43:32serhiy.storchakasetmessages: + msg246572
2015-07-10 03:24:44rhettingersetfiles: + intermediary.diff
2015-07-09 17:39:34serhiy.storchakasetmessages: + msg246513
2015-07-09 02:09:55serhiy.storchakasetmessages: + msg246477
2015-07-08 21:42:08vstinnersettitle: Crash when source set is changed during merging -> set.update(): Crash when source set is changed during merging
2015-07-08 21:40:38rhettingersetmessages: + msg246472
2015-07-08 20:13:30serhiy.storchakasetmessages: + msg246471
2015-07-08 20:13:12serhiy.storchakasetmessages: - msg246469
2015-07-08 20:11:19serhiy.storchakasetmessages: + msg246469
2015-07-08 16:17:43rhettingersetstage: needs patch -> patch review
2015-07-08 15:54:01rhettingersetfiles: + index_to_entry.diff
2015-07-07 22:52:47rhettingersetpriority: normal -> high
2015-07-07 22:52:03rhettingersetassignee: rhettinger
2015-07-07 11:56:10serhiy.storchakacreate