[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Berker Peksag added the comment: > 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!) -- nosy:

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

STINNER Victor added the comment: Should we backport the fix to Python 3.3 and 3.4 as well? I don't think so. -- ___ Python tracker

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Larry Hastings added the comment: 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 --

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

STINNER Victor added the comment: Thank you for fixes Naoki! -- ___ Python tracker ___ ___ Python-bugs-list

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: 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 --

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: 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 --

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: I opened backport PR for 3.6, 2.7 and 3.5. -- versions: +Python 2.7, Python 3.5 ___ Python tracker ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Changes by INADA Naoki : -- pull_requests: +3236 ___ Python tracker ___ ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Changes by INADA Naoki : -- pull_requests: +3235 ___ Python tracker ___ ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Changes by INADA Naoki : -- pull_requests: +3234 ___ Python tracker ___ ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: 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 --

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Alexander Mohr added the comment: my vote is yes due to the defaultdict issue. We were hitting this in our prod env -- ___ Python tracker ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

STINNER Victor added the comment: 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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Larry Hastings added the comment: 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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

STINNER Victor added the comment: 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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Larry Hastings added the comment: Victor, what do you think, does this need a 3.5 backport? I'm inclined to say yes. -- ___ Python tracker ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

STINNER Victor added the comment: > /* 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 ;-) -- ___ Python

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: BTW, should this backported to Python 3.5? -- nosy: +larry ___ Python tracker ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: > 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); --

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

STINNER Victor added the comment: "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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Changes by INADA Naoki : -- pull_requests: +3019 ___ Python tracker ___ ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Alexander Mohr added the comment: omg I just realized I need the default dict one too, great investigation work! -- ___ Python tracker ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: Docs/extending/newtypes.rst and Docs/include/noddy3.c should be updated too. But I'm not good English writer. I need help. -- ___ Python tracker

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: > 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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Alexander Mohr added the comment: 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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Alexander Mohr added the comment: I suggest any places that don't need the calls should have comments so that future reviewers know why. -- ___ Python tracker

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Changes by Serhiy Storchaka : -- nosy: +serhiy.storchaka type: -> crash ___ Python tracker ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: # _json module scanner_dealloc() encoder_dealloc() # _struct module unpackiter_dealloc # _ssl module context_dealloc() # Objects/ setiter_dealloc dictiter_dealloc dictview_dealloc # Parser/ ast_dealloc -- ___

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: > 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. -- ___ Python tracker

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

INADA Naoki added the comment: # 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

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

Alexander Mohr added the comment: should the base method which calls tp_dealloc do this? Maybe can kill all birds with one stone. -- nosy: +thehesiod ___ Python tracker

[issue31095] Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC

New submission from INADA Naoki: 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. -- components: Extension