classification
Title: Checking all tp_dealloc with Py_TPFLAGS_HAVE_GC
Type: crash Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, inada.naoki, larry, serhiy.storchaka, thehesiod, vstinner
Priority: normal Keywords:

Created on 2017-08-01 05:49 by inada.naoki, last changed 2017-10-28 22:47 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
lru_cache_segv.py inada.naoki, 2017-08-01 05:49
Pull Requests
URL Status Linked Edit
PR 2974 merged inada.naoki, 2017-08-01 09:52
PR 3195 merged inada.naoki, 2017-08-24 06:03
PR 3196 merged inada.naoki, 2017-08-24 06:10
PR 3197 merged inada.naoki, 2017-08-24 07:04
Messages (27)
msg299600 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) 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) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) Date: 2017-08-22 06:21
BTW, should this backported to Python 3.5?
msg300683 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) Date: 2017-08-24 07:05
I opened backport PR for 3.6, 2.7 and 3.5.
msg301203 - (view) Author: Inada Naoki (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) 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) * (Python committer) Date: 2017-09-04 04:41
Thank you for fixes Naoki!
msg303080 - (view) Author: Larry Hastings (larry) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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!)
History
Date User Action Args
2017-10-28 22:47:25berker.peksagsetstatus: open -> closed

nosy: + berker.peksag
messages: + msg305169

resolution: fixed
stage: resolved
2017-09-27 08:23:58vstinnersetmessages: + msg303113
2017-09-26 21:24:22larrysetmessages: + msg303080
2017-09-04 04:41:53vstinnersetmessages: + msg301208
2017-09-04 03:31:43inada.naokisetmessages: + msg301204
2017-09-04 03:31:11inada.naokisetmessages: + msg301203
2017-08-24 07:05:20inada.naokisetmessages: + msg300778
versions: + Python 2.7, Python 3.5
2017-08-24 07:04:21inada.naokisetpull_requests: + pull_request3236
2017-08-24 06:10:34inada.naokisetpull_requests: + pull_request3235
2017-08-24 06:03:09inada.naokisetpull_requests: + pull_request3234
2017-08-24 05:55:20inada.naokisetmessages: + msg300776
2017-08-24 01:36:07thehesiodsetmessages: + msg300772
2017-08-22 22:56:48vstinnersetmessages: + msg300728
2017-08-22 17:31:52larrysetmessages: + msg300715
2017-08-22 17:24:01vstinnersetmessages: + msg300714
2017-08-22 17:11:57larrysetmessages: + msg300712
2017-08-22 09:42:53vstinnersetmessages: + msg300683
2017-08-22 06:21:19inada.naokisetnosy: + larry
messages: + msg300669
2017-08-22 06:17:05inada.naokisetmessages: + msg300668
2017-08-21 11:11:53vstinnersetnosy: + vstinner
messages: + msg300621
2017-08-01 09:52:31inada.naokisetpull_requests: + pull_request3019
2017-08-01 09:18:10thehesiodsetmessages: + msg299615
2017-08-01 08:59:48inada.naokisetmessages: + msg299614
2017-08-01 08:52:02inada.naokisetmessages: + msg299613
2017-08-01 08:36:35thehesiodsetmessages: + msg299612
2017-08-01 08:16:13thehesiodsetmessages: + msg299611
2017-08-01 07:27:16serhiy.storchakasetnosy: + serhiy.storchaka
type: crash
2017-08-01 06:57:57inada.naokisetmessages: + msg299608
2017-08-01 06:30:08inada.naokisetmessages: + msg299605
2017-08-01 06:26:03inada.naokisetmessages: + msg299604
2017-08-01 05:51:58thehesiodsetnosy: + thehesiod
messages: + msg299601
2017-08-01 05:49:49inada.naokicreate