Do not use NFI to call finalizeCAPI in VM shutdown hook

Author: fangerer

graalpython/com.oracle.graal.python.cext/src/capi.c

@@ -456,7 +456,7 @@ PyAPI_FUNC(Py_ssize_t) PyTruffle_bulk_DEALLOC(intptr_t ptrArray[], int64_t len)
 
 /** to be used from Java code only and only at exit; calls _Py_Dealloc */
 PyAPI_FUNC(Py_ssize_t) PyTruffle_shutdown_bulk_DEALLOC(intptr_t ptrArray[], int64_t len) {
-    /* some objects depends on others which might get deallocated in the process 
+    /* some objects depends on others which might get deallocated in the process
         of an earlier deallocation of the other object. To avoid double deallocations,
         we, temporarly, make all objects immortal artificially */
 	for (int i = 0; i < len; i++) {
@@ -883,12 +883,41 @@ So we rebind them to no-ops when exiting.
 Py_ssize_t nop_GraalPy_get_PyObject_ob_refcnt(PyObject* obj) {
  return 100; // large dummy refcount
 }
+
 void nop_GraalPy_set_PyObject_ob_refcnt(PyObject* obj, Py_ssize_t refcnt) {
  // do nothing
 }
-PyAPI_FUNC(void) finalizeCAPI() {
- GraalPy_get_PyObject_ob_refcnt = nop_GraalPy_get_PyObject_ob_refcnt;
- GraalPy_set_PyObject_ob_refcnt = nop_GraalPy_set_PyObject_ob_refcnt;
+
+/*
+ * This array contains pairs of variable address and "reset value".
+ * The variable location is usually the address of a function pointer variable
+ * and the reset value is a new value to set at VM shutdown.
+ * For further explanation why this is required, see Java method
+ * 'CApiContext.ensureCapiWasLoaded'.
+ *
+ * Array format: [ var_addr, reset_val, var_addr1, reset_val1, ..., NULL ]
+ *
+ * ATTENTION: If the structure of the array's content is changed, method
+ *            'CApiContext.addNativeFinalizer' *MUST BE* adopted.
+ *
+ * ATTENTION: the array is expected to be NULL-terminated !
+ *
+ */
+static int64_t reset_func_ptrs[] = {
+        &GraalPy_get_PyObject_ob_refcnt,
+        nop_GraalPy_get_PyObject_ob_refcnt,
+        &GraalPy_set_PyObject_ob_refcnt,
+        nop_GraalPy_set_PyObject_ob_refcnt,
+        /* sentinel (required) */
+        NULL
+};
+
+/*
+ * This function is called from Java during C API initialization to get the
+ * pointer to array 'reset_func_pts'.
+ */
+PyAPI_FUNC(int64_t *) GraalPy_get_finalize_capi_pointer_array() {
+    return reset_func_ptrs;
 }
 
 static void unimplemented(const char* name) {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiContext.java

@@ -108,7 +108,6 @@
 import com.oracle.truffle.api.TruffleLogger;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.interop.ArityException;
-import com.oracle.truffle.api.interop.InteropException;
 import com.oracle.truffle.api.interop.InteropLibrary;
 import com.oracle.truffle.api.interop.TruffleObject;
 import com.oracle.truffle.api.interop.UnknownIdentifierException;
@@ -125,6 +124,8 @@
 import com.oracle.truffle.api.strings.TruffleString;
 import com.oracle.truffle.nfi.api.SignatureLibrary;
 
+import sun.misc.Unsafe;
+
 public final class CApiContext extends CExtContext {
 
     public static final String LOGGER_CAPI_NAME = "capi";
@@ -593,10 +594,11 @@ public static CApiContext ensureCapiWasLoaded(Node node, PythonContext context,
                      * is terminated by a signal, the context exit is skipped. For that case we set
                      * up the shutdown hook.
                      */
-                    Object finalizeFunction = U.readMember(capiLibrary, "finalizeCAPI");
-                    Object finalizeSignature = env.parseInternal(Source.newBuilder(J_NFI_LANGUAGE, "():VOID", "exec").build()).call();
+                    Object finalizeFunction = U.readMember(capiLibrary, "GraalPy_get_finalize_capi_pointer_array");
+                    Object finalizeSignature = env.parseInternal(Source.newBuilder(J_NFI_LANGUAGE, "():POINTER", "exec").build()).call();
+                    Object resetFunctionPointerArray = SignatureLibrary.getUncached().call(finalizeSignature, finalizeFunction);
                     try {
-                        cApiContext.addNativeFinalizer(env, finalizeFunction, finalizeSignature);
+                        cApiContext.addNativeFinalizer(env, resetFunctionPointerArray);
                     } catch (RuntimeException e) {
                         // This can happen when other languages restrict multithreading
                         LOGGER.warning(() -> "didn't register a native finalizer due to: " + e.getMessage());
@@ -622,16 +624,40 @@ public static CApiContext ensureCapiWasLoaded(Node node, PythonContext context,
         return context.getCApiContext();
     }
 
-    private void addNativeFinalizer(Env env, Object finalizeFunction, Object finalizeSignature) {
-        nativeFinalizerRunnable = () -> {
+    /**
+     * Registers a VM shutdown hook, that resets certain C API function to NOP functions to avoid
+     * segfaults due to improper Python context shutdown. In particular, the shutdown hook reads
+     * pairs of {@code variable address} and {@code reset value} provided in a native array and
+     * writes the {@code reset value} to the {@code variable address}. Currently, this is used to
+     * change the incref and decref upcall functions which would crash if they are called after the
+     * Python context was closed. The format of the array is:
+     * 
+     * <pre>
+     *     [ var_addr, reset_val, var_addr1, reset_val1, ..., NULL ]
+     * </pre>
+     */
+    private void addNativeFinalizer(Env env, Object resetFunctionPointerArray) {
+        final Unsafe unsafe = getContext().getUnsafe();
+        InteropLibrary lib = InteropLibrary.getUncached(resetFunctionPointerArray);
+        if (!lib.isNull(resetFunctionPointerArray) && lib.isPointer(resetFunctionPointerArray)) {
             try {
-                SignatureLibrary.getUncached().call(finalizeSignature, finalizeFunction);
-            } catch (InteropException e) {
-                throw CompilerDirectives.shouldNotReachHere(e);
+                long lresetFunctionPointerArray = lib.asPointer(resetFunctionPointerArray);
+                nativeFinalizerRunnable = () -> {
+                    long resetFunctionPointerLocation;
+                    long curElemAddr = lresetFunctionPointerArray;
+                    while ((resetFunctionPointerLocation = unsafe.getLong(curElemAddr)) != 0) {
+                        curElemAddr += Long.BYTES;
+                        long replacingFunctionPointer = unsafe.getLong(curElemAddr);
+                        unsafe.putAddress(resetFunctionPointerLocation, replacingFunctionPointer);
+                        curElemAddr += Long.BYTES;
+                    }
+                };
+                nativeFinalizerShutdownHook = env.newTruffleThreadBuilder(nativeFinalizerRunnable).build();
+                Runtime.getRuntime().addShutdownHook(nativeFinalizerShutdownHook);
+            } catch (UnsupportedMessageException e) {
+                throw new RuntimeException(e);
             }
-        };
-        nativeFinalizerShutdownHook = env.newTruffleThreadBuilder(nativeFinalizerRunnable).build();
-        Runtime.getRuntime().addShutdownHook(nativeFinalizerShutdownHook);
+        }
     }
 
     @TruffleBoundary