| msg299600 - (view) |
Author: Inada Naoki (methane) *  |
Date: 2017-08-01 05:49 |
| like GH-2966, most types with Py_TPFLAGS_HAVE_GC should call PyObject_GC_UnTrack() at top of the tp_dealloc. For example, I found lru_cache doesn't call it and I can produce segmentation fault. I'll check other types too. |
|
|
| msg299601 - (view) |
Author: Alexander Mohr (thehesiod) * |
Date: 2017-08-01 05:51 |
| should the base method which calls tp_dealloc do this? Maybe can kill all birds with one stone. |
|
|
| msg299604 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-01 06:26 |
| # collection module dequeiter_dealloc doesn't call Untrack(), but it's safe because it only frees deque and deque_dealloc calls Untrack() dequeiter_dealloc(dequeiterobject *dio) { Py_XDECREF(dio->deque); defdict_dealloc doesn't call Untrack(). And it can cause segfault: from collections import defaultdict import gc class Evil: def __del__(self): gc.collect() def __call__(self): return 42 def main(): d = defaultdict(Evil()) for i in range(1000): print(i) main() # _elementtree module elementiter_dealloc() calls UnTrack(), but it seems too late? static void elementiter_dealloc(ElementIterObject *it) { Py_ssize_t i = it->parent_stack_used; it->parent_stack_used = 0; while (i--) Py_XDECREF(it->parent_stack[i].parent); PyMem_Free(it->parent_stack); Py_XDECREF(it->sought_tag); Py_XDECREF(it->root_element); PyObject_GC_UnTrack(it); PyObject_GC_Del(it); } |
|
|
| msg299605 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-01 06:30 |
| > dequeiter_dealloc doesn't call Untrack(), but it's safe because it only frees deque > and deque_dealloc calls Untrack() It may be not true, while I don't have exploit yet. |
|
|
| msg299608 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-01 06:57 |
| # _json module scanner_dealloc() encoder_dealloc() # _struct module unpackiter_dealloc # _ssl module context_dealloc() # Objects/ setiter_dealloc dictiter_dealloc dictview_dealloc # Parser/ ast_dealloc |
|
|
| msg299611 - (view) |
Author: Alexander Mohr (thehesiod) * |
Date: 2017-08-01 08:16 |
| I suggest any places that don't need the calls should have comments so that future reviewers know why. |
|
|
| msg299612 - (view) |
Author: Alexander Mohr (thehesiod) * |
Date: 2017-08-01 08:36 |
| actually another idea: could the PR for this also update https://docs.python.org/2/c-api/typeobj.html#c.PyTypeObject.tp_dealloc to mention about these macros and when they should be used? That, along with all the other locations correctly calling these macros, and having comments when they're not needed hopefully should prevent this from happening again. |
|
|
| msg299613 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-01 08:52 |
| > should the base method which calls tp_dealloc do this? Maybe can kill all birds with one stone. It may be possible, but unclear. Object finalization process is very complicated. I agree that UnTrack object as soon as refcnt=0, and Track only when resurrected seems clearer. But changing such basic object system needs carefully designed by expert. |
|
|
| msg299614 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-01 08:59 |
| Docs/extending/newtypes.rst and Docs/include/noddy3.c should be updated too. But I'm not good English writer. I need help. |
|
|
| msg299615 - (view) |
Author: Alexander Mohr (thehesiod) * |
Date: 2017-08-01 09:18 |
| omg I just realized I need the default dict one too, great investigation work! |
|
|
| msg300621 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-08-21 11:11 |
| "like GH-2966, most types with Py_TPFLAGS_HAVE_GC should call PyObject_GC_UnTrack() at top of the tp_dealloc." Hum, I wasn't aware of that. Writing correctly code for the Python garbage collector is very complex :-/ Maybe it would help to have a short comment, maybe with a link to this issue, on each PyObject_GC_UnTrack(). At the first read, I saw the newly added PyObject_GC_UnTrack() calls as duplicate, and so useless. For example, PyObject_Del() already untracks the object, so it doesn't seem to be needed to explicitly call PyObject_GC_UnTrack() "just before". |
|
|
| msg300668 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-22 06:17 |
| > Maybe it would help to have a short comment, maybe with a link to this issue, on each PyObject_GC_UnTrack(). Is this looks good to you? /* UnTrack is needed before calling any callbacks (bpo-31095) */ PyObject_GC_UnTrack(self); |
|
|
| msg300669 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-22 06:21 |
| BTW, should this backported to Python 3.5? |
|
|
| msg300683 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-08-22 09:42 |
| > /* UnTrack is needed before calling any callbacks (bpo-31095) */ LGTM. I suggest /* bpo-31095: UnTrack is needed before calling any callbacks */ which is a little bit shorter, but it's up to you ;-) |
|
|
| msg300712 - (view) |
Author: Larry Hastings (larry) * |
Date: 2017-08-22 17:11 |
| Victor, what do you think, does this need a 3.5 backport? I'm inclined to say yes. |
|
|
| msg300714 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-08-22 17:24 |
| Larry Hastings: "Victor, what do you think, does this need a 3.5 backport? I'm inclined to say yes." Naoki has to design an evil object which triggers explicitly the garbage collector to get a crash. He found the bug by reading the code. I don't remind anyone complaining about the bug. So I don't think that it's a major bug, as was bpo-21435 which was *easy* to trigger using asyncio. So no, I don't think that this issue desevers a backport. But it's just my opinion, feel free to backport to 3.5 if you consider that the bug is critical enough ;-) |
|
|
| msg300715 - (view) |
Author: Larry Hastings (larry) * |
Date: 2017-08-22 17:31 |
| I thought crashing bugs were generally considered security bugs. With a reliable crashing bug exploiting a reasonable module (e.g. not ctypes) you can crash Python instances in hosting services. If those instances are shared with other users (e.g. Google App Engine) you can cause a temporary DOS. At least, that's the theory as I understood it...! |
|
|
| msg300728 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-08-22 22:56 |
| In my experience, it's not that hard to crash CPython (without ctypes) if you know internals or just if you look into Lib/test/crashers/ :-) I agree that it's a good practice to fix crashes when there is a simple known script to crash Python. The question is more where is the limit between security and bug fixes. To be honest, the change is very safe and is very short. @Larry: It's up to you, you are the release manager :-) If we consider that the discussed bugs impact the security, I will ask for backports to 2.7, 3.3 and 3.4 as well. |
|
|
| msg300772 - (view) |
Author: Alexander Mohr (thehesiod) * |
Date: 2017-08-24 01:36 |
| my vote is yes due to the defaultdict issue. We were hitting this in our prod env |
|
|
| msg300776 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-24 05:55 |
| New changeset a6296d34a478b4f697ea9db798146195075d496c by INADA Naoki in branch 'master': bpo-31095: fix potential crash during GC (GH-2974) https://github.com/python/cpython/commit/a6296d34a478b4f697ea9db798146195075d496c |
|
|
| msg300778 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-08-24 07:05 |
| I opened backport PR for 3.6, 2.7 and 3.5. |
|
|
| msg301203 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-09-04 03:31 |
| New changeset 2eea952b1b9ebbc2d94fd3faca1536c6b4963725 by INADA Naoki in branch '3.6': bpo-31095: fix potential crash during GC (GH-3195) https://github.com/python/cpython/commit/2eea952b1b9ebbc2d94fd3faca1536c6b4963725 |
|
|
| msg301204 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-09-04 03:31 |
| New changeset 4cde4bdcc86cb08ee3847500a172cc24eba37ffe by INADA Naoki in branch '2.7': bpo-31095: Fix potential crash during GC (GH-3197) https://github.com/python/cpython/commit/4cde4bdcc86cb08ee3847500a172cc24eba37ffe |
|
|
| msg301208 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-09-04 04:41 |
| Thank you for fixes Naoki! |
|
|
| msg303080 - (view) |
Author: Larry Hastings (larry) * |
Date: 2017-09-26 21:24 |
| New changeset 0fcc03367b31f44c1e1b8d3d2dd940ef1e744326 by larryhastings (INADA Naoki) in branch '3.5': bpo-31095: fix potential crash during GC (GH-2974) (#3196) https://github.com/python/cpython/commit/0fcc03367b31f44c1e1b8d3d2dd940ef1e744326 |
|
|
| msg303113 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-09-27 08:23 |
| Should we backport the fix to Python 3.3 and 3.4 as well? I don't think so. |
|
|
| msg305169 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2017-10-28 22:47 |
| > Should we backport the fix to Python 3.3 and 3.4 as well? > > I don't think so. I agree with Victor. Closing this as all PRs have been merged. Thank you, all (especially for the documentation update!) |
|
|