zopefoundation / zope.securitypolicy

Default Security Policy for Zope 3
Other
2 stars 3 forks source link

ZopeSecurityPolicy can lead to a fatal stack overflow #8

Open jamadden opened 7 years ago

jamadden commented 7 years ago

Spotted running the functional tests of zope.app.apidoc. Clearly there's a cycle involved in the __parent__ that apidoc sets up, which is not good, but crashing doesn't seem like the right response.

Fatal Python error: Cannot recover from stack overflow.

Current thread 0x00007fffafddd3c0 (most recent call first):
  File "//lib/python3.5/site-packages/zope/interface/declarations.py", line 177 in __hash__
  File "//lib/python3.5/weakref.py", line 197 in get
  File "//lib/python3.5/site-packages/zope/interface/declarations.py", line 559 in Provides
  File "//lib/python3.5/site-packages/zope/interface/declarations.py", line 804 in ObjectSpecification
  File "//lib/python3.5/site-packages/zope/proxy/decorator.py", line 39 in __get__
  File "//lib/python3.5/site-packages/zope/location/location.py", line 109 in __getattribute__
  File "//lib/python3.5/site-packages/zope/component/hooks.py", line 119 in adapter_hook
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 136 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
  File "//lib/python3.5/site-packages/zope/securitypolicy/zopepolicy.py", line 145 in cached_prinper
agroszer commented 7 years ago

I think if you have cycles in __parent__ that will shoot you anyway badly in the foot. You could sprinkle protection against that all over the place, e.g. in absoluteURL, etc.

mgedmin commented 7 years ago

It think it's fine if a __parent__ loop triggers RuntimeError: maximum recursion depth exceeded.

I don't think it's fine if it kills the entire Python process with a fatal error in C code somewhere.

jamadden commented 7 years ago

Here's the C stack of the crash:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib          0x00007fffa6ff9d42 __pthread_kill + 10
1   libsystem_pthread.dylib         0x00007fffa70e75bf pthread_kill + 90
2   libsystem_c.dylib               0x00007fffa6f5f420 abort + 129
3   org.python.python               0x0000000107b772d1 Py_FatalError + 462
4   org.python.python               0x0000000107b54545 _Py_CheckRecursiveCall + 65
5   org.python.python               0x0000000107ac299c PyObject_Call + 86
6   org.python.python               0x0000000107ac2b55 call_function_tail + 72
7   org.python.python               0x0000000107ac2db0 callmethod + 122
8   org.python.python               0x0000000107ac2782 _PyObject_CallMethodId + 169
9   org.python.python               0x0000000107b77d94 flush_std_files + 87
10  org.python.python               0x0000000107b772cc Py_FatalError + 457
11  org.python.python               0x0000000107b54545 _Py_CheckRecursiveCall + 65
12  org.python.python               0x0000000107ac299c PyObject_Call + 86
13  org.python.python               0x0000000107b5cd0f PyEval_CallObjectWithKeywords + 165
14  org.python.python               0x0000000107ad65ed wrapperdescr_call + 130
15  org.python.python               0x0000000107ac29ab PyObject_Call + 101
16  org.python.python               0x0000000107b59cdd PyEval_EvalFrameEx + 22197
17  org.python.python               0x0000000107b5d4c9 _PyEval_EvalCodeWithName + 1886
18  org.python.python               0x0000000107b54613 PyEval_EvalCodeEx + 78
19  org.python.python               0x0000000107ae25f8 function_call + 369
20  org.python.python               0x0000000107ac29ab PyObject_Call + 101
21  org.python.python               0x0000000107ad2bdd method_call + 140
22  org.python.python               0x0000000107ac29ab PyObject_Call + 101
23  org.python.python               0x0000000107b5cd0f PyEval_CallObjectWithKeywords + 165
24  org.python.python               0x0000000107b0a8f7 slot_tp_hash + 51
25  org.python.python               0x0000000107b050cd tuplehash + 65
26  org.python.python               0x0000000107af2450 dict_subscript + 47

So it looks like Python detected the recursion at 11, attempted to flush the standard streams (9) and found they were actually Python objects that it needed to call (because this happened in a doctest), detected the recursion again (4) and finally aborted.

Now the C code from CheckRecursiveCall looks like this:

    if (tstate->recursion_critical)
        /* Somebody asked that we don't check for recursion. */
        return 0;
    if (tstate->overflowed) {
        if (tstate->recursion_depth > recursion_limit + 50) {
            /* Overflowing while handling an overflow. Give up. */
            Py_FatalError("Cannot recover from stack overflow.");
        }
        return 0;
    }
    if (tstate->recursion_depth > recursion_limit) {
        --tstate->recursion_depth;
        tstate->overflowed = 1;
        PyErr_Format(PyExc_RecursionError,
                     "maximum recursion depth exceeded%s",
                     where);
        return -1;
    }
    return 0;

If I read that correctly, that means that the first CheckRecursiveCall at 11 shouldn't have directly called Py_FatalError; there must be a handler somewhere that caught RecursionError and did complicated enough handling to get us back here---but I don't see that on either stack.