Merge pull request #324 from python-greenlet/noexcept-crashing-on-linux

Linux: Fix a rare crash that could occur when shutting down an  interpreter running multiple threads

8 files changed, 161 insertions, 59 deletions

diff --git a/CHANGES.rst b/CHANGES.rst
index b2d69b9..49bfaf9 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -5,7 +5,9 @@
 2.0.0rc4 (unreleased)
 =====================
 
-- Nothing changed yet.
+- Linux: Fix a rare crash that could occur when shutting down an
+  interpreter running multiple threads, when some of those threads are
+  in greenlets making calls to functions that release the GIL.
 
 
 2.0.0rc3 (2022-10-29)
diff --git a/appveyor.yml b/appveyor.yml
index bbe8226..307cf99 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -36,17 +36,15 @@ environment:
 
   matrix:
     # http://www.appveyor.com/docs/installed-software#python
+
+    # Fully supported 64-bit versions, with testing. This should be
+    # all the current (non EOL) versions.
     - PYTHON: "C:\\Python311-x64"
       PYTHON_VERSION: "3.11.0"
       PYTHON_ARCH: "64"
       PYTHON_EXE: python
       APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
 
-    - PYTHON: "C:\\Python27"
-      PYTHON_ARCH: "32"
-      PYTHON_VERSION: "2.7.x"
-      PYTHON_EXE: python
-
     - PYTHON: "C:\\Python310-x64"
       PYTHON_VERSION: "3.10.0"
       PYTHON_ARCH: "64"
@@ -59,9 +57,15 @@ environment:
       PYTHON_EXE: python
       APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
 
-    - PYTHON: "C:\\Python39"
-      PYTHON_ARCH: "32"
-      PYTHON_VERSION: "3.9.x"
+    - PYTHON: "C:\\Python38-x64"
+      PYTHON_ARCH: "64"
+      PYTHON_VERSION: "3.8.x"
+      PYTHON_EXE: python
+      APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
+
+    - PYTHON: "C:\\Python37-x64"
+      PYTHON_ARCH: "64"
+      PYTHON_VERSION: "3.7.x"
       PYTHON_EXE: python
       APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
 
@@ -70,45 +74,65 @@ environment:
       PYTHON_VERSION: "2.7.x"
       PYTHON_EXE: python
 
-    - PYTHON: "C:\\Python35"
+    # Tested 32-bit versions. A small, hand-picked selection covering
+    # important variations. No need to include newer versions of
+    # cpython here, 32-bit x86 windows is on the way out.
+
+    - PYTHON: "C:\\Python39"
       PYTHON_ARCH: "32"
-      PYTHON_VERSION: "3.5.x"
+      PYTHON_VERSION: "3.9.x"
       PYTHON_EXE: python
+      APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
 
-    - PYTHON: "C:\\Python35-x64"
-      PYTHON_ARCH: "64"
-      PYTHON_VERSION: "3.5.x"
+    - PYTHON: "C:\\Python27"
+      PYTHON_ARCH: "32"
+      PYTHON_VERSION: "2.7.x"
       PYTHON_EXE: python
 
-    - PYTHON: "C:\\Python37"
-      PYTHON_ARCH: "32"
-      PYTHON_VERSION: "3.7.x"
+
+    # Untested 64-bit versions. We don't expect any variance here from
+    # the other tested 64-bit versions, OR they are very EOL
+
+    - PYTHON: "C:\\Python36-x64"
+      PYTHON_ARCH: "64"
+      PYTHON_VERSION: "3.6.x"
       PYTHON_EXE: python
+      GWHEEL_ONLY: true
 
-    - PYTHON: "C:\\Python37-x64"
+    - PYTHON: "C:\\Python35-x64"
       PYTHON_ARCH: "64"
-      PYTHON_VERSION: "3.7.x"
+      PYTHON_VERSION: "3.5.x"
       PYTHON_EXE: python
+      GWHEEL_ONLY: true
+
+    # Untested 32-bit versions. As above, we don't expect any variance
+    # from the tested 32-bit versions, OR they are very EOL.
 
     - PYTHON: "C:\\Python38"
       PYTHON_ARCH: "32"
       PYTHON_VERSION: "3.8.x"
       PYTHON_EXE: python
+      GWHEEL_ONLY: true
 
-    - PYTHON: "C:\\Python38-x64"
-      PYTHON_ARCH: "64"
-      PYTHON_VERSION: "3.8.x"
+    - PYTHON: "C:\\Python37"
+      PYTHON_ARCH: "32"
+      PYTHON_VERSION: "3.7.x"
       PYTHON_EXE: python
+      GWHEEL_ONLY: true
 
     - PYTHON: "C:\\Python36"
       PYTHON_ARCH: "32"
       PYTHON_VERSION: "3.6.x"
       PYTHON_EXE: python
+      GWHEEL_ONLY: true
 
-    - PYTHON: "C:\\Python36-x64"
-      PYTHON_ARCH: "64"
-      PYTHON_VERSION: "3.6.x"
+    - PYTHON: "C:\\Python35"
+      PYTHON_ARCH: "32"
+      PYTHON_VERSION: "3.5.x"
       PYTHON_EXE: python
+      GWHEEL_ONLY: true
+
+
 
 cache:
   - "%TMP%\\py\\"
@@ -177,7 +201,7 @@ build_script:
 
 test_script:
   - "%CMD_IN_ENV% python -c \"import faulthandler; assert faulthandler.is_enabled()\""
-  - "%CMD_IN_ENV% python -m zope.testrunner --test-path=src -vvv"
+  -  if not "%GWHEEL_ONLY%"=="true" %PYEXE% -m zope.testrunner --test-path=src -vvv
 # XXX: Doctest disabled pending sphinx release for 3.10; see tests.yml.
 #  - "%CMD_IN_ENV% python -m sphinx -b doctest -d docs/_build/doctrees docs docs/_build/doctest"
 
diff --git a/setup.py b/setup.py
index f73ba0a..a4248af 100755
--- a/setup.py
+++ b/setup.py
@@ -22,6 +22,8 @@ cpp_link_args = []
 # Extra compiler arguments passed to the main extension
 main_compile_args = []
 
+is_win = sys.platform.startswith("win")
+
 # workaround segfaults on openbsd and RHEL 3 / CentOS 3 . see
 # https://bitbucket.org/ambroff/greenlet/issue/11/segfault-on-openbsd-i386
 # https://github.com/python-greenlet/greenlet/issues/4
@@ -39,7 +41,7 @@ if ((sys.platform == "openbsd4" and os.uname()[-1] == "i386")
 if sys.platform == 'darwin':
     # The clang compiler doesn't use --std=c++11 by default
     cpp_compile_args.append("--std=gnu++11")
-elif sys.platform == 'win32' and "MSC" in platform.python_compiler():
+elif is_win and "MSC" in platform.python_compiler():
     # Older versions of MSVC (Python 2.7) don't handle C++ exceptions
     # correctly by default. While newer versions do handle exceptions by default,
     # they don't do it fully correctly. So we need an argument on all versions.
@@ -49,10 +51,13 @@ elif sys.platform == 'win32' and "MSC" in platform.python_compiler():
     # OR
     #   "a" == standard C++, and Windows SEH; anything may throw, compiler optimizations
     #          around try blocks are less aggressive.
-    # /EHsc is suggested, as /EHa isn't supposed to be linked to other things not built
-    # with it.
+    # /EHsc is suggested, and /EHa isn't supposed to be linked to other things not built
+    # with it. Leaving off the "c" should just result in slower, safer code.
+    # Other options:
+    #    "r" == Always generate standard confirming checks for noexcept blocks, terminating
+    #           if violated. IMPORTANT: We rely on this.
     # See https://docs.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model?view=msvc-160
-    handler = "/EHsc"
+    handler = "/EHsr"
     cpp_compile_args.append(handler)
     # To disable most optimizations:
     #cpp_compile_args.append('/Od')
@@ -100,7 +105,7 @@ else:
 
     headers = [GREENLET_HEADER]
 
-    if sys.platform == 'win32' and '64 bit' in sys.version:
+    if is_win and '64 bit' in sys.version:
         # this works when building with msvc, not with 64 bit gcc
         # switch_<platform>_masm.obj can be created with setup_switch_<platform>_masm.cmd
         obj_fn = 'switch_arm64_masm.obj' if platform.machine() == 'ARM64' else 'switch_x64_masm.obj'
@@ -108,7 +113,7 @@ else:
     else:
         extra_objects = []
 
-    if sys.platform == 'win32' and os.environ.get('GREENLET_STATIC_RUNTIME') in ('1', 'yes'):
+    if is_win and os.environ.get('GREENLET_STATIC_RUNTIME') in ('1', 'yes'):
         main_compile_args.append('/MT')
     elif hasattr(os, 'uname') and os.uname()[4] in ['ppc64el', 'ppc64le']:
         main_compile_args.append('-fno-tree-dominator-opts')
@@ -126,7 +131,12 @@ else:
             depends=[
                 GREENLET_HEADER,
                 GREENLET_SRC_DIR + 'slp_platformselect.h',
-            ] + _find_platform_headers() + _find_impl_headers()
+            ] + _find_platform_headers() + _find_impl_headers(),
+            define_macros=[
+            ] + ([
+                ('WIN32', '1'),
+            ] if is_win else [
+            ])
         ),
         # Test extensions.
         #
diff --git a/src/greenlet/greenlet.cpp b/src/greenlet/greenlet.cpp
index 4e46d42..7777e9f 100644
--- a/src/greenlet/greenlet.cpp
+++ b/src/greenlet/greenlet.cpp
@@ -6,6 +6,7 @@
  * Fix missing braces with:
  *   clang-tidy src/greenlet/greenlet.c -fix -checks="readability-braces-around-statements"
 */
+#include <cstdlib>
 #include <string>
 #include <algorithm>
 #include <exception>
@@ -1269,7 +1270,7 @@ UserGreenlet::g_initialstub(void* mark)
        explicitly to us. Either way, the ``err`` variable is
        created twice at the same memory location, but possibly
        having different ``origin`` values. Note that it's not
-       constructed for the second time until the switch actually happens.`
+       constructed for the second time until the switch actually happens.
     */
     if (err.status == 1) {
         // This never returns!
@@ -1296,7 +1297,7 @@ UserGreenlet::g_initialstub(void* mark)
 
 
 void
-UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) G_NOEXCEPT
+UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) G_NOEXCEPT_WIN32
 {
     // The arguments here would be another great place for move.
     // As it is, we take them as a reference so that when we clear
@@ -1369,25 +1370,47 @@ UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run)
         catch(...) {
             // Unhandled C++ exception!
 
-            // If we don't catch this here, most platforms will just
-            // abort() the process. But on 64-bit Windows with older
-            // versions of the C runtime, this can actually corrupt
-            // memory and just return. We see this when compiling with
-            // the Windows 7.0 SDK targeting Windows Server 2008, but
-            // not when using the Appveyor Visual Studio 2019 image.
-            // So this currently only affects Python 2.7 on Windows
-            // 64. That is, the tests pass and the runtime aborts
+            // If we declare ourselves as noexcept, if we don't catch
+            // this here, most platforms will just abort() the
+            // process. But on 64-bit Windows with older versions of
+            // the C runtime, this can actually corrupt memory and
+            // just return. We see this when compiling with the
+            // Windows 7.0 SDK targeting Windows Server 2008, but not
+            // when using the Appveyor Visual Studio 2019 image. So
+            // this currently only affects Python 2.7 on Windows 64.
+            // That is, the tests pass and the runtime aborts
             // everywhere else.
             //
             // However, if we catch it and try to continue with a
             // Python error, then all Windows 64 bit platforms corrupt
-            // memory. So all we can do is manually abort.
+            // memory. So all we can do is manually abort, hopefully
+            // with a good error message. (Note that the above was
+            // tested WITHOUT the `/EHr` switch being used at compile
+            // time, so MSVC may have "optimized" out important
+            // checking. Using that switch, we may be in a better
+            // place in terms of memory corruption.) But sometimes it
+            // can't be caught here at all, which is confusing but not
+            // terribly surprising; so again, the G_NOEXCEPT_WIN32
+            // plus "/EHr".
             //
             // Hopefully the basic C stdlib is still functional enough
             // for us to at least print an error.
-            fprintf(stderr, "greenlet: Unhandled C++ exception from a greenlet run function. ");
-            fprintf(stderr, "Because memory is likely corrupted, terminating process.");
-            abort();
+            //
+            // It gets more complicated than that, though, on some
+            // platforms, specifically at least Linux/gcc/libstdc++. They use
+            // an exception to unwind the stack when a background
+            // thread exits. (See comments about G_NOEXCEPT.) So this
+            // may not actually represent anything untoward. On those
+            // platforms we allow throws of this to propagate, or
+            // attempt to anyway.
+# if defined(WIN32) || defined(_WIN32)
+            Py_FatalError(
+                "greenlet: Unhandled C++ exception from a greenlet run function. "
+                "Because memory is likely corrupted, terminating process.");
+            std::abort();
+#else
+            throw;
+#endif
         }
     }
     args.CLEAR();
@@ -1443,6 +1466,7 @@ UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run)
     PyErr_WriteUnraisable(this->self().borrow_o());
     Py_FatalError("greenlet: ran out of parent greenlets while propagating exception; "
                   "cannot continue");
+    std::abort();
 }
 
 
diff --git a/src/greenlet/greenlet_compiler_compat.hpp b/src/greenlet/greenlet_compiler_compat.hpp
index bf11813..ecaeb32 100644
--- a/src/greenlet/greenlet_compiler_compat.hpp
+++ b/src/greenlet/greenlet_compiler_compat.hpp
@@ -5,6 +5,35 @@
 /**
  * Definitions to aid with compatibility with different compilers.
  *
+ * .. caution:: Use extreme care with G_NOEXCEPT.
+ * Some compilers and runtimes, specifically gcc/libgcc/libstdc++ on
+ * Linux, implement stack unwinding by throwing an uncatchable
+ * exception, one that specifically does not appear to be an active
+ * exception to the rest of the runtime. If this happens while we're in a G_NOEXCEPT function,
+ * we have violated our dynamic exception contract, and so the runtime
+ * will call std::terminate(), which kills the process with the
+ * unhelpful message "terminate called without an active exception".
+ *
+ * This has happened in this scenario: A background thread is running
+ * a greenlet that has made a native call and released the GIL.
+ * Meanwhile, the main thread finishes and starts shutting down the
+ * interpreter. When the background thread is scheduled again and
+ * attempts to obtain the  GIL, it notices that the interpreter is
+ * exiting and calls ``pthread_exit()``. This in turn starts to unwind
+ * the stack by throwing that exception. But we had the ``PyCall``
+ * functions annotated as G_NOEXCEPT, so the runtime terminated us.
+ *
+ * #2  0x00007fab26fec2b7 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6
+ * #3  0x00007fab26febb3c in __gxx_personality_v0 () from /lib/x86_64-linux-gnu/libstdc++.so.6
+ * #4  0x00007fab26f34de6 in ?? () from /lib/x86_64-linux-gnu/libgcc_s.so.1
+ * #6  0x00007fab276a34c6 in __GI___pthread_unwind  at ./nptl/unwind.c:130
+ * #7  0x00007fab2769bd3a in __do_cancel () at ../sysdeps/nptl/pthreadP.h:280
+ * #8  __GI___pthread_exit (value=value@entry=0x0) at ./nptl/pthread_exit.c:36
+ * #9  0x000000000052e567 in PyThread_exit_thread () at ../Python/thread_pthread.h:370
+ * #10 0x00000000004d60b5 in take_gil at ../Python/ceval_gil.h:224
+ * #11 0x00000000004d65f9 in PyEval_RestoreThread  at ../Python/ceval.c:467
+ * #12 0x000000000060cce3 in setipaddr  at ../Modules/socketmodule.c:1203
+ * #13 0x00000000006101cd in socket_gethostbyname
  */
 
 
@@ -93,5 +122,11 @@ typedef unsigned int uint32_t;
 #    define UNUSED(x) UNUSED_ ## x
 #endif
 
+#if defined(_MSC_VER)
+#    define G_NOEXCEPT_WIN32 G_NOEXCEPT
+#else
+#    define G_NOEXCEPT_WIN32
+#endif
+
 
 #endif
diff --git a/src/greenlet/greenlet_greenlet.hpp b/src/greenlet/greenlet_greenlet.hpp
index 1b8a83f..292fce4 100644
--- a/src/greenlet/greenlet_greenlet.hpp
+++ b/src/greenlet/greenlet_greenlet.hpp
@@ -589,7 +589,7 @@ namespace greenlet
     protected:
         virtual switchstack_result_t g_initialstub(void* mark);
     private:
-        void inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) G_NOEXCEPT;
+        void inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) G_NOEXCEPT_WIN32;
     };
 
     class MainGreenlet : public Greenlet
diff --git a/src/greenlet/greenlet_refs.hpp b/src/greenlet/greenlet_refs.hpp
index 622880b..160b9e4 100644
--- a/src/greenlet/greenlet_refs.hpp
+++ b/src/greenlet/greenlet_refs.hpp
@@ -223,14 +223,14 @@ namespace greenlet {
         inline OwnedObject PyGetAttr(const ImmortalObject& name) const G_NOEXCEPT;
         inline OwnedObject PyRequireAttr(const char* const name) const;
         inline OwnedObject PyRequireAttr(const ImmortalObject& name) const;
-        inline OwnedObject PyCall(const BorrowedObject& arg) const G_NOEXCEPT;
-        inline OwnedObject PyCall(PyGreenlet* arg) const G_NOEXCEPT;
-        inline OwnedObject PyCall(const PyObject* arg) const G_NOEXCEPT;
+        inline OwnedObject PyCall(const BorrowedObject& arg) const;
+        inline OwnedObject PyCall(PyGreenlet* arg) const ;
+        inline OwnedObject PyCall(const PyObject* arg) const ;
         // PyObject_Call(this, args, kwargs);
         inline OwnedObject PyCall(const BorrowedObject args,
-                                  const BorrowedObject kwargs) const G_NOEXCEPT;
+                                  const BorrowedObject kwargs) const;
         inline OwnedObject PyCall(const OwnedObject& args,
-                                  const OwnedObject& kwargs) const G_NOEXCEPT;
+                                  const OwnedObject& kwargs) const;
 
     protected:
         void _set_raw_pointer(void* t)
@@ -706,19 +706,19 @@ namespace greenlet {
     }
 
     template<typename T, TypeChecker TC>
-    inline OwnedObject PyObjectPointer<T, TC>::PyCall(const BorrowedObject& arg) const G_NOEXCEPT
+    inline OwnedObject PyObjectPointer<T, TC>::PyCall(const BorrowedObject& arg) const
     {
         return this->PyCall(arg.borrow());
     }
 
     template<typename T, TypeChecker TC>
-    inline OwnedObject PyObjectPointer<T, TC>::PyCall(PyGreenlet* arg) const G_NOEXCEPT
+    inline OwnedObject PyObjectPointer<T, TC>::PyCall(PyGreenlet* arg) const
     {
         return this->PyCall(reinterpret_cast<const PyObject*>(arg));
     }
 
     template<typename T, TypeChecker TC>
-    inline OwnedObject PyObjectPointer<T, TC>::PyCall(const PyObject* arg) const G_NOEXCEPT
+    inline OwnedObject PyObjectPointer<T, TC>::PyCall(const PyObject* arg) const
     {
         assert(this->p);
         return OwnedObject::consuming(PyObject_CallFunctionObjArgs(this->p, arg, NULL));
@@ -726,7 +726,7 @@ namespace greenlet {
 
     template<typename T, TypeChecker TC>
     inline OwnedObject PyObjectPointer<T, TC>::PyCall(const BorrowedObject args,
-                                                  const BorrowedObject kwargs) const G_NOEXCEPT
+                                                  const BorrowedObject kwargs) const
     {
         assert(this->p);
         return OwnedObject::consuming(PyObject_Call(this->p, args, kwargs));
@@ -734,7 +734,7 @@ namespace greenlet {
 
     template<typename T, TypeChecker TC>
     inline OwnedObject PyObjectPointer<T, TC>::PyCall(const OwnedObject& args,
-                                                  const OwnedObject& kwargs) const G_NOEXCEPT
+                                                  const OwnedObject& kwargs) const
     {
         assert(this->p);
         return OwnedObject::consuming(PyObject_Call(this->p, args.borrow(), kwargs.borrow()));
diff --git a/src/greenlet/tests/_test_extension_cpp.cpp b/src/greenlet/tests/_test_extension_cpp.cpp
index d9d7f38..d4dfd5e 100644
--- a/src/greenlet/tests/_test_extension_cpp.cpp
+++ b/src/greenlet/tests/_test_extension_cpp.cpp
@@ -110,14 +110,21 @@ test_exception_switch_and_do_in_g2(PyObject* self, PyObject* args)
             return NULL;
         }
     }
-    catch (...) {
+    catch (const exception_t& e) {
         /* if we are here the memory can be already corrupted and the program
          * might crash before below py-level exception might become printed.
          * -> print something to stderr to make it clear that we had entered
          *    this catch block.
+         * See comments in inner_bootstrap()
          */
+#if defined(WIN32) || defined(_WIN32)
         fprintf(stderr, "C++ exception unexpectedly caught in g1\n");
         PyErr_SetString(PyExc_AssertionError, "C++ exception unexpectedly caught in g1");
+        Py_XDECREF(result);
+        return NULL;
+#else
+        throw;
+#endif
     }
 
     Py_XDECREF(result);