From e99220a3e44fb93b382c43bdcd3f1d36c87a3a0e Mon Sep 17 00:00:00 2001 From: Ivan FB <128452529+Ivansete-status@users.noreply.github.com> Date: Tue, 26 May 2026 23:46:27 +0200 Subject: [PATCH] fix use-after-free concern (#47) Co-authored-by: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com> --- ffi/ffi_context.nim | 111 ++++++++++++++++------------- tests/unit/test_event_dispatch.nim | 76 +++++++++++++++++++- 2 files changed, 138 insertions(+), 49 deletions(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 5ddd5a9..800b88e 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -78,43 +78,54 @@ const git_version* {.strdefine.} = "n/a" template callEventCallback*(ctx: ptr FFIContext, eventName: string, body: untyped) = ## `body` may evaluate to a `string` or a `seq[byte]` — the cast to ## `ptr cchar` accepts both `ptr char` and `ptr byte` source pointers. - let (cbPtr, ud) = snapshotCallback(ctx[].callbackState) - if isNil(cbPtr): - chronicles.error eventName & " - eventCallback is nil" - return - - foreignThreadGc: - let cb = cast[FFICallBack](cbPtr) - try: - let event = body - cb(RET_OK, cast[ptr cchar](unsafeAddr event[0]), cast[csize_t](len(event)), ud) - except Exception, CatchableError: - let msg = - "Exception " & eventName & " when calling 'eventCallBack': " & - getCurrentExceptionMsg() - cb(RET_ERR, cast[ptr cchar](unsafeAddr msg[0]), cast[csize_t](len(msg)), ud) + ## + ## Holds `callbackState.lock` for the snapshot + invocation so that a + ## concurrent `setCallback` from a foreign thread blocks until the + ## in-flight callback returns. Without this, the foreign-side binding + ## could free the object `userData` points at between the snapshot and + ## the invocation, causing a use-after-free. + withLock ctx[].callbackState.lock: + let cbPtr = ctx[].callbackState.callback + let ud = ctx[].callbackState.userData + if isNil(cbPtr): + chronicles.error eventName & " - eventCallback is nil" + return + foreignThreadGc: + let cb = cast[FFICallBack](cbPtr) + try: + let event = body + cb(RET_OK, cast[ptr cchar](unsafeAddr event[0]), cast[csize_t](len(event)), ud) + except Exception, CatchableError: + let msg = + "Exception " & eventName & " when calling 'eventCallBack': " & + getCurrentExceptionMsg() + cb(RET_ERR, cast[ptr cchar](unsafeAddr msg[0]), cast[csize_t](len(msg)), ud) template dispatchFFIEvent*(eventName: string, body: untyped) = ## Dispatches an FFI event to the callback registered via `{libName}_set_event_callback`. ## `body` is evaluated lazily — only when a callback is registered. ## `body` may produce a `string` (legacy JSON style) or a `seq[byte]` (CBOR). ## Valid only on the FFI thread (i.e., inside {.ffi.} proc bodies and their async closures). + ## + ## Lock-during-invocation contract: see `callEventCallback`. let ffiState = ffiCurrentCallbackState if isNil(ffiState): chronicles.error eventName & " - event callback not set" return - let (cbPtr, ud) = snapshotCallback(ffiState[]) - if isNil(cbPtr): - chronicles.error eventName & " - event callback not set" - return - foreignThreadGc: - let cb = cast[FFICallBack](cbPtr) - try: - let event = body - cb(RET_OK, cast[ptr cchar](unsafeAddr event[0]), cast[csize_t](len(event)), ud) - except Exception, CatchableError: - let msg = "Exception dispatching " & eventName & ": " & getCurrentExceptionMsg() - cb(RET_ERR, cast[ptr cchar](unsafeAddr msg[0]), cast[csize_t](len(msg)), ud) + withLock ffiState[].lock: + let cbPtr = ffiState[].callback + let ud = ffiState[].userData + if isNil(cbPtr): + chronicles.error eventName & " - event callback not set" + return + foreignThreadGc: + let cb = cast[FFICallBack](cbPtr) + try: + let event = body + cb(RET_OK, cast[ptr cchar](unsafeAddr event[0]), cast[csize_t](len(event)), ud) + except Exception, CatchableError: + let msg = "Exception dispatching " & eventName & ": " & getCurrentExceptionMsg() + cb(RET_ERR, cast[ptr cchar](unsafeAddr msg[0]), cast[csize_t](len(msg)), ud) type EventEnvelope*[T] = object ## Standard wire shape for CBOR-encoded FFI events: @@ -138,32 +149,36 @@ template dispatchFFIEventCbor*(eventName: string, eventPayload: typed) = ## NB: the template parameter is intentionally named `eventPayload` ## rather than `payload` — Nim's template substitution would otherwise ## also replace the `payload:` field name inside `EventEnvelope`. + ## + ## Lock-during-invocation contract: see `callEventCallback`. let ffiState = ffiCurrentCallbackState if ffiState.isNil(): chronicles.error eventName & " - event callback not set" return - let (cbPtr, ud) = snapshotCallback(ffiState[]) - if cbPtr.isNil(): - chronicles.error eventName & " - event callback not set" - return - foreignThreadGc: - let cb = cast[FFICallBack](cbPtr) - try: - var (data, dataLen) = cborEncodeShared( - EventEnvelope[typeof(eventPayload)](eventType: eventName, payload: eventPayload) - ) - defer: - cborFreeShared(data) - cb(RET_OK, cast[ptr cchar](data), cast[csize_t](dataLen), ud) - except Exception, CatchableError: - # Catching `Exception` also catches Defects (OOM, overflow, ...) so - # the C caller always gets RET_OK/RET_ERR. Requires `--panics:off` - # (Nim's default; don't enable `--panics:on` for this lib). + withLock ffiState[].lock: + let cbPtr = ffiState[].callback + let ud = ffiState[].userData + if cbPtr.isNil(): + chronicles.error eventName & " - event callback not set" + return + foreignThreadGc: + let cb = cast[FFICallBack](cbPtr) + try: + var (data, dataLen) = cborEncodeShared( + EventEnvelope[typeof(eventPayload)](eventType: eventName, payload: eventPayload) + ) + defer: + cborFreeShared(data) + cb(RET_OK, cast[ptr cchar](data), cast[csize_t](dataLen), ud) + except Exception, CatchableError: + # Catching `Exception` also catches Defects (OOM, overflow, ...) so + # the C caller always gets RET_OK/RET_ERR. Requires `--panics:off` + # (Nim's default; don't enable `--panics:on` for this lib). - let msg = "Exception dispatching " & eventName & ": " & getCurrentExceptionMsg() - cb( - RET_ERR, cast[ptr cchar](unsafeAddr msg[0]), cast[csize_t](len(msg)), ud - ) + let msg = "Exception dispatching " & eventName & ": " & getCurrentExceptionMsg() + cb( + RET_ERR, cast[ptr cchar](unsafeAddr msg[0]), cast[csize_t](len(msg)), ud + ) proc sendRequestToFFIThread*( ctx: ptr FFIContext, ffiRequest: ptr FFIThreadRequest, timeout = InfiniteDuration diff --git a/tests/unit/test_event_dispatch.nim b/tests/unit/test_event_dispatch.nim index a12fa91..4e459cf 100644 --- a/tests/unit/test_event_dispatch.nim +++ b/tests/unit/test_event_dispatch.nim @@ -7,7 +7,7 @@ ## sendRequestToFFIThread) so we exercise the threadvar-backed ## ffiCurrentCallbackState wiring, not just the templates in isolation. -import std/[locks] +import std/[locks, os] import unittest2 import results import ffi @@ -238,3 +238,77 @@ suite "FFICallbackState concurrent access": # `evt` got hit by every dispatch above; just confirm at least one # actually landed so a silently-broken dispatch loop is caught. check evt.called + +# --------------------------------------------------------------------------- +# Lock-during-invocation regression (issue #40 second concern) +# --------------------------------------------------------------------------- + +## A foreign-thread `setCallback` must not be able to swap the callback + +## userData pair while an in-flight dispatch is mid-invocation on the +## previous pair. The dispatch templates now hold `callbackState.lock` +## for the entire snapshot + invocation, so `setCallback` blocks until +## dispatch returns. +## +## We don't use the FFI thread's request channel here because the request +## handler runs synchronously on the FFI thread before +## `reqReceivedSignal` fires — there'd be no way for the main thread to +## observe the in-flight state. Instead, a worker thread directly drives +## `dispatchFFIEvent` against a registry-of-one. + +type SlowState = object + entered: Atomic[bool] + exited: Atomic[bool] + +proc slowEventCb( + retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer +) {.cdecl, gcsafe, raises: [].} = + ## Signal entry, sleep briefly (the window during which the main + ## thread must call setCallback and block), signal exit. + let st = cast[ptr SlowState](userData) + st[].entered.store(true) + os.sleep(15) + st[].exited.store(true) + +type DispatcherArgs = tuple + state: ptr FFICallbackState + done: ptr Atomic[bool] + +proc dispatcherBody(args: DispatcherArgs) {.thread.} = + ffiCurrentCallbackState = args.state + dispatchFFIEvent("evt"): + "payload" + args.done[].store(true) + +suite "callbackState lock held during invocation": + test "setCallback blocks until in-flight dispatch finishes": + var state: FFICallbackState + initCallbackState(state) + defer: + deinitCallbackState(state) + + var st: SlowState + st.entered.store(false) + st.exited.store(false) + + setCallback(state, cast[pointer](slowEventCb), addr st) + + var done: Atomic[bool] + done.store(false) + var thr: Thread[DispatcherArgs] + createThread(thr, dispatcherBody, (addr state, addr done)) + + # Wait until the worker thread is inside slowEventCb. + for _ in 0 ..< 200: + if st.entered.load(): + break + os.sleep(1) + check st.entered.load() + check not st.exited.load() + + # Lock-during-invocation contract: setCallback blocks until the + # dispatch's lock is released, i.e. until slowEventCb has finished. + var other = SlowState() # dummy target; never invoked + setCallback(state, cast[pointer](slowEventCb), addr other) + check st.exited.load() + joinThread(thr) + check done.load()