mirror of
https://github.com/logos-messaging/nim-ffi.git
synced 2026-06-20 16:29:31 +00:00
fix use-after-free concern (#47)
Co-authored-by: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com>
This commit is contained in:
Ivan FB
GitHub
parent
e43c1e03e8
commit
e99220a3e4
@ -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
|
||||
|
||||
@ -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()
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user