| msg249625 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2015-09-03 09:53 |
| 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. |
|
|
| msg249626 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 09:55 |
| 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 |
|
|
| msg249639 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-09-03 13:57 |
| 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 (, ). |
|
|
| msg249640 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 14:03 |
| > 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). |
|
|
| msg249645 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-09-03 14:24 |
| > We only give the reference to the caller when the newly created OrderedDict > is fully initialized (consistent). See . PyDict_New() can trigger garbage collecting and traversing , and GC have a reference to underinitialized OrderedDict and can call odict_traverse() for it. |
|
|
| msg249648 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 14:37 |
| > See . 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. |
|
|
| msg249649 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 14:38 |
| @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? |
|
|
| msg249651 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-09-03 14:50 |
| 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. |
|
|
| msg249652 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 14:54 |
| > 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? |
|
|
| msg249656 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-09-03 15:15 |
| I left a nitpick. In any case the patch LGTM. |
|
|
| msg249663 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2015-09-03 15:51 |
| New changeset ef1f5aebe1a6 by Victor Stinner in branch '3.5': Issue #24992: Fix error handling and a race condition (related to garbage https://hg.python.org/cpython/rev/ef1f5aebe1a6 |
|
|
| msg249665 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 15:51 |
| > I left a nitpick. In any case the patch LGTM. Ok, fixed. I pushed my fix. Thanks for the review Serhiy. |
|
|
| msg249675 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2015-09-03 17:00 |
| Is this something that we should ship in 3.5.0rc3? |
|
|
| msg249682 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 19:16 |
| 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. |
|
|
| msg249702 - (view) |
Author: Eric Snow (eric.snow) * |
Date: 2015-09-04 00:10 |
| Thanks for taking care of this, Victor (and Serhiy). :) |
|
|