fix(c): register the calling thread with the GC on abi=c method entry (#146)

.github/workflows/ci.yml

@@ -80,6 +80,15 @@ jobs:
       nim-versions:   ${{ needs.versions.outputs.nim-versions }}
       nimble-version: ${{ needs.versions.outputs.nimble }}
 
+  foreign-thread-c-abi:
+    name: Foreign Thread (abi = c)
+    needs: versions
+    uses: ./.github/workflows/test.yml
+    with:
+      test: test_foreign_thread_c_abi
+      nim-versions:   ${{ needs.versions.outputs.nim-versions }}
+      nimble-version: ${{ needs.versions.outputs.nimble }}
+
   c-wire:
     # The `c`-ABI cwire codec is layout-/allocator-sensitive (malloc/free, flat
     # struct packing, allocShared for seq/Option), so cover it across the full

ffi/internal/c_macro_helpers.nim

@@ -711,8 +711,18 @@ proc ctxBindingGuard(
 ): NimNode {.compileTime.} =
   ## Prologue that binds `ctxIdent`: a method validates the ctx it was handed, a
   ## static resolves the library's shared one.
+  #
+  # Any call can be a host thread's first entry. The body allocates via the GC
+  # on the calling thread, so register it first; initializeLibrary is idempotent.
+  # Raw AST: `when declared` of an undeclared symbol inside `quote` ICEs.
+  let initGuard = nnkWhenStmt.newTree(
+    nnkElifBranch.newTree(
+      newCall(ident("declared"), ident("initializeLibrary")),
+      newStmtList(newCall(ident("initializeLibrary"))),
+    )
+  )
   if not isStatic:
-    return quote:
+    let methodGuard = quote:
       if onReply.isNil():
         return RET_MISSING_CALLBACK
       if not `poolIdent`.isValidCtx(cast[pointer](`ctxIdent`)):
@@ -720,6 +730,8 @@ proc ctxBindingGuard(
           RET_ERR, `emptyReply`, "ctx is not a valid FFI context".cstring, userData
         )
         return RET_ERR
+    methodGuard.insert(0, initGuard)
+    return methodGuard
   let guard = quote:
     if onReply.isNil():
       return RET_MISSING_CALLBACK
@@ -727,17 +739,7 @@ proc ctxBindingGuard(
       let errStr = "ffiStatic: " & error
       onReply(RET_ERR, `emptyReply`, errStr.cstring, userData)
       return RET_ERR
-  # A static call may be the host's first entry into the library. Raw AST, not
-  # `quote`: `when declared` over an undeclared symbol inside `quote` ICEs.
-  guard.insert(
-    0,
-    nnkWhenStmt.newTree(
-      nnkElifBranch.newTree(
-        newCall(ident("declared"), ident("initializeLibrary")),
-        newStmtList(newCall(ident("initializeLibrary"))),
-      )
-    ),
-  )
+  guard.insert(0, initGuard)
   guard
 
 proc exportedProc(
@@ -745,7 +747,9 @@ proc exportedProc(
     boxName, envWire, trampName, poolIdent, cbType: NimNode,
     isStatic: bool,
 ): NimNode =
-  # No `foreignThreadGc`: `cwireUnpack`/`cwirePack` alloc on the calling thread (already GC-registered); wrapping would free its live ORC heap.
+  # `cwireUnpack`/`cwirePack` alloc on the calling thread; `ctxBindingGuard`
+  # registered it. No teardown: it would free the heap of a host thread still
+  # calling in. A host thread that exits leaks its heap; accepted.
   let envName = spec.envelope
   let ctxIdent = ident("ctx")
   # String reply: empty non-nil cstring on error; object reply: nil ptr gated by err_code.

tests/unit/fixtures/foreign_thread_c_abi_fixture.nim

@@ -0,0 +1,210 @@
+## Fixture for test_foreign_thread_c_abi. It calls `abi = c` method entry
+## points from threads that the Nim runtime does not know.
+
+import std/[locks, strutils]
+import results
+import ffi
+
+type ThreadLib = object
+  tag: string
+
+# declareLibrary imports the NimMain symbol of the dylib. This fixture links as
+# an executable, so it must supply an empty stub.
+{.emit: "void libthreadedcabiNimMain(void) {}".}
+
+declareLibrary("threadedcabi", ThreadLib, defaultABIFormat = "c")
+
+type ThreadConfig {.ffi.} = object
+  tag: string
+
+proc threadedcabi_create*(
+    cfg: ThreadConfig
+): Future[Result[ThreadLib, string]] {.ffiCtor.} =
+  return ok(ThreadLib(tag: cfg.tag))
+
+proc threadedcabi_echo*(
+    lib: ThreadLib, text: string
+): Future[Result[string, string]] {.ffi.} =
+  ## The method takes a string, so the request unpack allocates GC-managed
+  ## memory on the calling thread. That is the operation under test.
+  return ok(lib.tag & ":" & text)
+
+genBindings()
+
+type ReplyData = object
+  lock: Lock
+  cond: Cond
+  called: bool
+  retCode: cint
+  text: string
+
+proc initReplyData(d: var ReplyData) =
+  d.lock.initLock()
+  d.cond.initCond()
+
+proc deinitReplyData(d: var ReplyData) =
+  d.cond.deinitCond()
+  d.lock.deinitLock()
+
+proc waitReply(d: var ReplyData) =
+  acquire(d.lock)
+  while not d.called:
+    wait(d.cond, d.lock)
+  release(d.lock)
+
+proc onStringReply(
+    err: cint, reply: cstring, errMsg: cstring, ud: pointer
+) {.cdecl, gcsafe, raises: [].} =
+  let d = cast[ptr ReplyData](ud)
+  acquire(d[].lock)
+  if err == RET_OK and not reply.isNil():
+    d[].text = $reply
+  d[].retCode = err
+  d[].called = true
+  signal(d[].cond)
+  release(d[].lock)
+
+proc packedWire[W, R](_: typedesc[W], envelope: R): W =
+  var wire: W
+  cwirePack(wire, envelope)
+  wire
+
+proc makeCtx(tag: string): ptr FFIContext[ThreadLib] =
+  var d: ReplyData
+  initReplyData(d)
+  defer:
+    deinitReplyData(d)
+
+  var wire = packedWire(
+    ThreadedcabiCreateCtorReq_CWire,
+    ThreadedcabiCreateCtorReq(cfg: ThreadConfig(tag: tag)),
+  )
+  defer:
+    cwireFree(wire)
+
+  doAssert not ThreadedcabiCreateCtorReqCAbiExport(addr wire, onStringReply, addr d)
+    .isNil()
+  waitReply(d)
+  doAssert d.retCode == RET_OK
+  # The ctor's reply text is the new ctx pointer as a decimal string.
+  cast[ptr FFIContext[ThreadLib]](cast[uint](parseBiggestUInt(d.text)))
+
+# Nim's createThread registers its new thread with the GC. A registered thread
+# cannot show the bug. The test thread must come from the platform API.
+{.
+  emit: """
+typedef int (*NimFfiEchoFn)(void*, void*, void*, const void*);
+
+typedef struct {
+  void* fn; void* ctx; void* cb; void* ud; const void* req; int ret;
+} NimFfiForeignCall;
+
+static void nimffi_foreign_body(NimFfiForeignCall* c) {
+  c->ret = ((NimFfiEchoFn)c->fn)(c->ctx, c->cb, c->ud, c->req);
+}
+"""
+.}
+
+when defined(windows):
+  {.
+    emit: """/*INCLUDESECTION*/
+#include <windows.h>
+"""
+  .}
+  {.
+    emit: """
+static DWORD WINAPI nimffi_foreign_thread_main(LPVOID arg) {
+  nimffi_foreign_body((NimFfiForeignCall*)arg);
+  return 0;
+}
+
+int nimffi_call_on_foreign_thread(
+    void* fn, void* ctx, void* cb, void* ud, const void* req) {
+  NimFfiForeignCall c;
+  HANDLE t;
+  c.fn = fn; c.ctx = ctx; c.cb = cb; c.ud = ud; c.req = req; c.ret = -1;
+  t = CreateThread(NULL, 0, nimffi_foreign_thread_main, &c, 0, NULL);
+  if (t == NULL) return -2;
+  WaitForSingleObject(t, INFINITE);
+  CloseHandle(t);
+  return c.ret;
+}
+"""
+  .}
+else:
+  {.
+    emit: """/*INCLUDESECTION*/
+#include <pthread.h>
+"""
+  .}
+  {.
+    emit: """
+static void* nimffi_foreign_thread_main(void* arg) {
+  nimffi_foreign_body((NimFfiForeignCall*)arg);
+  return (void*)0;
+}
+
+int nimffi_call_on_foreign_thread(
+    void* fn, void* ctx, void* cb, void* ud, const void* req) {
+  NimFfiForeignCall c;
+  pthread_t t;
+  c.fn = fn; c.ctx = ctx; c.cb = cb; c.ud = ud; c.req = req; c.ret = -1;
+  if (pthread_create(&t, (void*)0, nimffi_foreign_thread_main, &c) != 0) return -2;
+  pthread_join(t, (void*)0);
+  return c.ret;
+}
+"""
+  .}
+
+proc nimffi_call_on_foreign_thread(
+  fn, ctx, cb, ud: pointer, req: pointer
+): cint {.importc, nodecl.}
+
+proc callOnForeignThread(
+    ctx: ptr FFIContext[ThreadLib], req: ptr ThreadedcabiEchoReq_CWire, d: ptr ReplyData
+): cint =
+  ## This proc passes the export to C as an opaque pointer. Only the typedef
+  ## above can differ from the generated signature.
+  nimffi_call_on_foreign_thread(
+    cast[pointer](ThreadedcabiEchoReqCAbiExport),
+    cast[pointer](ctx),
+    cast[pointer](onStringReply),
+    cast[pointer](d),
+    cast[pointer](req),
+  )
+
+proc runScenario(tag: string, rounds: int): bool =
+  ## Creates one context, then makes `rounds` calls to it. Each call starts a
+  ## new platform thread, runs the entry point on it once, and joins it.
+  let ctx = makeCtx(tag)
+  for i in 0 ..< rounds:
+    var d: ReplyData
+    initReplyData(d)
+    var req =
+      packedWire(ThreadedcabiEchoReq_CWire, ThreadedcabiEchoReq(text: "call " & $i))
+
+    let rc = callOnForeignThread(ctx, addr req, addr d)
+    waitReply(d)
+    let good = rc == RET_OK and d.retCode == RET_OK and d.text == tag & ":call " & $i
+
+    cwireFree(req)
+    deinitReplyData(d)
+    if not good:
+      echo tag, ": round ", i, " failed: rc=", rc, " ret=", d.retCode, " text=", d.text
+      return false
+
+  if ThreadLibFFIPool.destroyFFIContext(ctx).isErr():
+    echo tag, ": destroyFFIContext failed"
+    return false
+  return true
+
+proc main(): int =
+  # Multiple rounds make sure that the registration is per-thread, not one
+  # time for the process.
+  if not runScenario("single", 1):
+    return 1
+  if not runScenario("multi", 8):
+    return 1
+  return 0
+
+quit(main())

tests/unit/test_foreign_thread_c_abi.nim

@@ -0,0 +1,43 @@
+## This test runs the fixture in a child process. The fixture calls `abi = c`
+## entry points from threads that the Nim runtime does not know. A regression
+## crashes the child, not this suite. The child uses the same --mm switch as
+## this run. The regression is fatal under refc and harmless under orc.
+
+import std/[os, osproc, compilesettings]
+import unittest2
+
+const
+  fixture =
+    currentSourcePath().parentDir() / "fixtures" / "foreign_thread_c_abi_fixture.nim"
+  nimExe = getCurrentCompilerExe()
+  ffiSearchPaths = querySettingSeq(searchPaths)
+  mmFlag =
+    when compileOption("mm", "refc"):
+      "--mm:refc"
+    elif compileOption("mm", "orc"):
+      "--mm:orc"
+    elif compileOption("mm", "arc"):
+      "--mm:arc"
+    else:
+      ""
+
+proc runFixture(): tuple[output: string, exitCode: int] =
+  let outDir = getTempDir() / "ffi_foreign_thread_out"
+  let cacheDir = getTempDir() / "ffi_foreign_thread_cache"
+  createDir(outDir)
+  var cmd = quoteShell(nimExe) & " c -r --hints:off --warnings:off"
+  if mmFlag.len > 0:
+    cmd.add(" " & mmFlag)
+  for p in ffiSearchPaths:
+    cmd.add(" --path:" & quoteShell(p))
+  cmd.add(" --nimcache:" & quoteShell(cacheDir))
+  # Write the binary to the temp directory. The fixture directory contains only source.
+  cmd.add(" --outdir:" & quoteShell(outDir))
+  cmd.add(" " & quoteShell(fixture))
+  execCmdEx(cmd)
+
+suite "abi = c entry points are callable from foreign host threads":
+  test "method calls from unregistered host threads succeed":
+    let (output, code) = runFixture()
+    checkpoint(output)
+    check code == 0