fix: segfault, where a python callback fields would be GCed by JS while (#85)

* fix segfault, where a python callback fields would be GCed by JS while
the callback is still alive

* remove unrrelated comment

* fix lint

* try fix ci

* try fix ci

Author: sigmaSd

.github/workflows/checks.yml

@@ -47,24 +47,18 @@ jobs:
       - name: Setup latest deno version
         uses: denoland/setup-deno@v2
 
+      - name: Setup Python
+        uses: actions/setup-python@v5
+        with:
+          python-version: "3.13"
+
       - name: Setup Bun
         if: ${{ matrix.os != 'windows-latest' }}
         uses: oven-sh/setup-bun@v1
 
-      - name: Setup Python (Windows)
-        uses: actions/setup-python@v2
-        if: ${{ matrix.os == 'windows-latest' }}
-        with:
-          python-version: "3.13"
-
       - name: Install NumPy
-        if: ${{ matrix.os != 'macos-latest' }}
         run: python3 -m pip install numpy
 
-      - name: Install NumPy on MacOs
-        if: ${{ matrix.os == 'macos-latest' }}
-        run: python3 -m pip install --user --break-system-packages numpy
-
       - name: Run deno test
         run: deno task test
 

deno.json

@@ -10,7 +10,7 @@
     "check:mod": "deno check --unstable-ffi mod.ts",
     "check:ext": "deno check --unstable-ffi ext/*.ts",
     "check:examples": "deno check --unstable-ffi examples/*.ts",
-    "test": "deno test --unstable-ffi -A test/test.ts",
+    "test": "deno test --unstable-ffi -A test/test.ts && deno test -A --v8-flags=--expose-gc test/test_with_gc.ts",
     "example:hello_python": "deno run -A --unstable-ffi examples/hello_python.ts",
     "example:matplotlib": "deno run -A --unstable-ffi examples/matplotlib.ts",
     "example:pip_import": "deno run -A --unstable-ffi examples/pip_import.ts",

ext/pip.ts

@@ -1,3 +1,4 @@
+// deno-lint-ignore-file no-import-prefix
 import { kw, python, PythonError } from "../mod.ts";
 
 import { join } from "jsr:@std/path@^1/join";

src/python.ts

@@ -143,6 +143,14 @@ export class Callback {
     result: "pointer";
   }>;
 
+  // Keep native-facing buffers alive for as long as this Callback exists
+  /** @private */
+  _pyMethodDef?: Uint8Array<ArrayBuffer>;
+  /** @private */
+  _nameBuf?: Uint8Array<ArrayBuffer>;
+  /** @private */
+  _docBuf?: Uint8Array<ArrayBuffer>;
+
   constructor(public callback: PythonJSCallback) {
     this.unsafe = new Deno.UnsafeCallback(
       {
@@ -490,49 +498,56 @@ export class PyObject {
         } else if (v instanceof Callback) {
           // https://docs.python.org/3/c-api/structures.html#c.PyMethodDef
           // there are extra 4 bytes of padding after ml_flags field
-          const pyMethodDef = new Uint8Array(8 + 8 + 4 + 4 + 8);
-          const view = new DataView(pyMethodDef.buffer);
+          // Build and pin PyMethodDef + string buffers on the Callback instance
+          const methodDef = new Uint8Array(8 + 8 + 4 + 4 + 8);
+          v._pyMethodDef = methodDef;
+          const view = new DataView(methodDef.buffer);
           const LE =
             new Uint8Array(new Uint32Array([0x12345678]).buffer)[0] !== 0x7;
 
           const name = "JSCallback:" + (v.callback.name || "anonymous");
-          const nameBuf = new TextEncoder().encode(`${name}\0`);
+          v._nameBuf = new TextEncoder().encode(`${name}\0`);
           view.setBigUint64(
             0,
-            BigInt(Deno.UnsafePointer.value(Deno.UnsafePointer.of(nameBuf)!)),
+            BigInt(
+              Deno.UnsafePointer.value(Deno.UnsafePointer.of(v._nameBuf)!),
+            ),
             LE,
           );
           view.setBigUint64(
             8,
             BigInt(Deno.UnsafePointer.value(v.unsafe.pointer)),
             LE,
           );
+          // METH_VARARGS | METH_KEYWORDS
           view.setInt32(16, 0x1 | 0x2, LE);
+
           // https://github.com/python/cpython/blob/f27593a87c344f3774ca73644a11cbd5614007ef/Objects/typeobject.c#L688
           const SIGNATURE_END_MARKER = ")\n--\n\n";
           // We're not using the correct arguments name, but just using dummy ones (because they're not accessible in js)
           const fnArgs = [...Array(v.callback.length).keys()]
             .map((_, i) => String.fromCharCode(97 + i)).join(",");
-          const docBuf = `${name}(${fnArgs}${SIGNATURE_END_MARKER}\0`;
+          v._docBuf = new TextEncoder().encode(
+            `${name}(${fnArgs}${SIGNATURE_END_MARKER}\0`,
+          );
           view.setBigUint64(
             24,
             BigInt(
-              Deno.UnsafePointer.value(
-                Deno.UnsafePointer.of(new TextEncoder().encode(docBuf))!,
-              ),
+              Deno.UnsafePointer.value(Deno.UnsafePointer.of(v._docBuf)!),
             ),
             LE,
           );
           const fn = py.PyCFunction_NewEx(
-            pyMethodDef,
+            v._pyMethodDef,
             PyObject.from(null).handle,
             null,
           );
 
           // NOTE: we need to extend `pyMethodDef` lifetime
           // Otherwise V8 can release it before the callback is called
+          // Is this still needed (after the change of pinning fields to the callabck) ? might be
           const pyObject = new PyObject(fn);
-          pyObject.#pyMethodDef = pyMethodDef;
+          pyObject.#pyMethodDef = methodDef;
           return pyObject;
         } else if (v instanceof PyObject) {
           return v;

src/util.ts

@@ -42,7 +42,7 @@ export function postSetup(lib: string) {
 /**
  * Encodes a C string.
  */
-export function cstr(str: string): Uint8Array {
+export function cstr(str: string): Uint8Array<ArrayBuffer> {
   const buf = new Uint8Array(str.length + 1);
   encoder.encodeInto(str, buf);
   return buf;

test/test_with_gc.ts

@@ -0,0 +1,48 @@
+import python, { Callback } from "../mod.ts";
+import { assertEquals } from "./asserts.ts";
+
+Deno.test("callbacks are not gc'd while still needed by python", () => {
+  const pyModule = python.runModule(
+    `
+stored_callback = None
+
+def store_and_call_callback(cb):
+  global stored_callback
+  stored_callback = cb
+  return stored_callback()
+
+def call_stored_callback():
+  global stored_callback
+  if stored_callback is None:
+    return -1
+  return stored_callback()
+  `,
+    "test_gc_module",
+  );
+
+  let callCount = 0;
+  const callback = () => {
+    callCount++;
+    return callCount * 10;
+  };
+
+  // Store the callback in Python and call it
+  const callbackObj = new Callback(callback);
+  assertEquals(pyModule.store_and_call_callback(callbackObj).valueOf(), 10);
+  assertEquals(callCount, 1);
+
+  for (let i = 0; i < 10; i++) {
+    // @ts-ignore:requires: --v8-flags=--expose-gc
+    gc();
+  }
+
+  // If the callback was incorrectly GC'd, this should segfault
+  // But it should work because Python holds a reference
+  assertEquals(pyModule.call_stored_callback().valueOf(), 20);
+  assertEquals(callCount, 2);
+
+  // Call it again to be sure
+  assertEquals(pyModule.call_stored_callback().valueOf(), 30);
+  assertEquals(callCount, 3);
+  callbackObj.destroy();
+});