From df2277e7266237f2c359992910dfa65483a27359 Mon Sep 17 00:00:00 2001 From: Ivan FB <128452529+Ivansete-status@users.noreply.github.com> Date: Mon, 27 Apr 2026 21:22:45 +0200 Subject: [PATCH] Fix memleaks (#11) * protect against mem leak in case of failures sending requests to ffi thread * better cleanup if failures in createFFIContext * avoid dangling cstring in handleRes under ARC/ORC * better resource cleanup in destroyFFIContext * invoke onNotResponding if failure in destroyFFIContext * correct seq copy in alloc * make sure the lock is init before cleanUpResources * better possible exception handling in processReq * guard allocSharedSeq if given seq is empty * enhance error handling in ffi_context * add new tests and some corrections --- ffi.nimble | 20 ++++-- ffi/alloc.nim | 10 ++- ffi/ffi_context.nim | 89 +++++++++++++++++++----- ffi/ffi_thread_request.nim | 16 +++-- ffi/internal/ffi_macro.nim | 7 +- tests/test_alloc.nim | 71 +++++++++++++++++++ tests/test_ffi_context.nim | 137 +++++++++++++++++++++++++++++++++++++ 7 files changed, 315 insertions(+), 35 deletions(-) create mode 100644 tests/test_alloc.nim create mode 100644 tests/test_ffi_context.nim diff --git a/ffi.nimble b/ffi.nimble index dc39f4c..9639a5c 100644 --- a/ffi.nimble +++ b/ffi.nimble @@ -12,11 +12,17 @@ requires "chronos" requires "chronicles" requires "taskpools" -# Source files to include -# srcDir = "src" -# installFiles = @["src/ffi.nim", "mylib.h"] +const nimFlags = "--mm:orc -d:chronicles_log_level=WARN" -# # 💡 Custom build step before installation -# before install: -# echo "Generating custom C header..." -# exec "nim r tools/gen_header.nim" +task build, "Compile the library": + exec "nim c " & nimFlags & " --noMain ffi.nim" + +task test, "Run all tests": + exec "nim c -r " & nimFlags & " tests/test_alloc.nim" + exec "nim c -r " & nimFlags & " tests/test_ffi_context.nim" + +task test_alloc, "Run alloc unit tests": + exec "nim c -r " & nimFlags & " tests/test_alloc.nim" + +task test_ffi, "Run FFI context integration tests": + exec "nim c -r " & nimFlags & " tests/test_ffi_context.nim" diff --git a/ffi/alloc.nim b/ffi/alloc.nim index 1a6f118..a12d5f9 100644 --- a/ffi/alloc.nim +++ b/ffi/alloc.nim @@ -25,12 +25,16 @@ proc alloc*(str: string): cstring = proc allocSharedSeq*[T](s: seq[T]): SharedSeq[T] = let data = allocShared(sizeof(T) * s.len) - if s.len != 0: - copyMem(data, unsafeAddr s[0], s.len) + + if s.len == 0: + return (cast[ptr UncheckedArray[T]](nil), 0) + + copyMem(data, unsafeAddr s[0], sizeof(T) * s.len) return (cast[ptr UncheckedArray[T]](data), s.len) proc deallocSharedSeq*[T](s: var SharedSeq[T]) = - deallocShared(s.data) + if not s.data.isNil(): + deallocShared(s.data) s.len = 0 proc toSeq*[T](s: SharedSeq[T]): seq[T] = diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index f110425..545d2d0 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -60,18 +60,23 @@ proc sendRequestToFFIThread*( ## Sending the request let sentOk = ctx.reqChannel.trySend(ffiRequest) if not sentOk: + deleteRequest(ffiRequest) return err("Couldn't send a request to the ffi thread") let fireSyncRes = ctx.reqSignal.fireSync() if fireSyncRes.isErr(): + deleteRequest(ffiRequest) return err("failed fireSync: " & $fireSyncRes.error) if fireSyncRes.get() == false: + deleteRequest(ffiRequest) return err("Couldn't fireSync in time") ## wait until the FFI working thread properly received the request let res = ctx.reqReceivedSignal.waitSync(timeout) if res.isErr(): + ## Do not free ffiRequest here: the FFI thread was already signaled and + ## will process (and free) it. return err("Couldn't receive reqReceivedSignal signal") ## Notice that in case of "ok", the deallocShared(req) is performed by the FFI Thread in the @@ -122,8 +127,14 @@ proc watchdogThreadBody(ctx: ptr FFIContext) {.thread.} = trace "Sending watchdog request to FFI thread" - sendRequestToFFIThread(ctx, WatchdogReq.ffiNewReq(callback, nilUserData), WatchdogTimeout).isOkOr: - error "Failed to send watchdog request to FFI thread", error = $error + try: + sendRequestToFFIThread( + ctx, WatchdogReq.ffiNewReq(callback, nilUserData), WatchdogTimeout + ).isOkOr: + error "Failed to send watchdog request to FFI thread", error = $error + onNotResponding(ctx) + except Exception as exc: + error "Exception sending watchdog request", exc = exc.msg onNotResponding(ctx) waitFor watchdogRun(ctx) @@ -138,13 +149,30 @@ proc processRequest[T]( ## The registeredRequests represents a table defined at compile time. ## Then, registeredRequests == Table[reqId, proc-handling-the-request-asynchronously] + ## Explicit conversion keeps `reqId` alive as the backing string, + ## avoiding the implicit string→cstring warning that will become an error. + let reqIdCs = reqId.cstring + let retFut = - if not ctx[].registeredRequests[].contains(reqId): + if not ctx[].registeredRequests[].contains(reqIdCs): ## That shouldn't happen because only registered requests should be sent to the FFI thread. nilProcess(request[].reqId) else: - ctx[].registeredRequests[][reqId](request[].reqContent, ctx) - handleRes(await retFut, request) + ctx[].registeredRequests[][reqIdCs](request[].reqContent, ctx) + + let res = + try: + await retFut + except CatchableError as exc: + Result[string, string].err("Exception in processRequest for " & reqId & ": " & exc.msg) + + ## handleRes may raise (OOM, GC setup) even though it is rare. Catching here + ## keeps the async proc raises:[] compatible. The defer inside handleRes + ## guarantees request is freed before the exception propagates. + try: + handleRes(res, request) + except Exception as exc: + error "Unexpected exception in handleRes", exc = exc.msg proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = ## FFI thread body that attends library user API requests @@ -180,15 +208,33 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = waitFor ffiRun(ctx) +proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = + defer: + freeShared(ctx) + ctx.lock.deinitLock() + if not ctx.reqSignal.isNil(): + ?ctx.reqSignal.close() + if not ctx.reqReceivedSignal.isNil(): + ?ctx.reqReceivedSignal.close() + return ok() + proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = ## This proc is called from the main thread and it creates ## the FFI working thread. var ctx = createShared(FFIContext[T], 1) - ctx.reqSignal = ThreadSignalPtr.new().valueOr: - return err("couldn't create reqSignal ThreadSignalPtr") - ctx.reqReceivedSignal = ThreadSignalPtr.new().valueOr: - return err("couldn't create reqReceivedSignal ThreadSignalPtr") ctx.lock.initLock() + + ctx.reqSignal = ThreadSignalPtr.new().valueOr: + ctx.cleanUpResources().isOkOr: + return err("could not clean resources in a failure new reqSignal: " & $error) + return err("couldn't create reqSignal ThreadSignalPtr: " & $error) + + ctx.reqReceivedSignal = ThreadSignalPtr.new().valueOr: + ctx.cleanUpResources().isOkOr: + return + err("could not clean resources in a failure new reqReceivedSignal: " & $error) + return err("couldn't create reqReceivedSignal ThreadSignalPtr") + ctx.registeredRequests = addr ffi_types.registeredRequests ctx.running.store(true) @@ -196,32 +242,39 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = try: createThread(ctx.ffiThread, ffiThreadBody[T], ctx) except ValueError, ResourceExhaustedError: - freeShared(ctx) + ctx.cleanUpResources().isOkOr: + error "failed to clean up resources after ffiThread creation failure", err = error return err("failed to create the FFI thread: " & getCurrentExceptionMsg()) try: createThread(ctx.watchdogThread, watchdogThreadBody, ctx) except ValueError, ResourceExhaustedError: - freeShared(ctx) + ctx.running.store(false) + let fireRes = ctx.reqSignal.fireSync() + if fireRes.isErr(): + error "failed to signal ffiThread during watchdog cleanup", err = fireRes.error + joinThread(ctx.ffiThread) + ctx.cleanUpResources().isOkOr: + error "failed to clean up resources after watchdogThread creation failure", err = error return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) return ok(ctx) proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) + defer: + joinThread(ctx.ffiThread) + joinThread(ctx.watchdogThread) + ctx.cleanUpResources().isOkOr: + error "failed to clean up resources in destroyFFIContext", err = error let signaledOnTime = ctx.reqSignal.fireSync().valueOr: + ctx.onNotResponding() return err("error in destroyFFIContext: " & $error) if not signaledOnTime: + ctx.onNotResponding() return err("failed to signal reqSignal on time in destroyFFIContext") - joinThread(ctx.ffiThread) - joinThread(ctx.watchdogThread) - ctx.lock.deinitLock() - ?ctx.reqSignal.close() - ?ctx.reqReceivedSignal.close() - freeShared(ctx) - return ok() template checkParams*(ctx: ptr FFIContext, callback: FFICallBack, userData: pointer) = diff --git a/ffi/ffi_thread_request.nim b/ffi/ffi_thread_request.nim index 4f3c119..7cdda61 100644 --- a/ffi/ffi_thread_request.nim +++ b/ffi/ffi_thread_request.nim @@ -6,11 +6,16 @@ import std/[json, macros], results, tables import chronos, chronos/threadsync import ./ffi_types, ./internal/ffi_macro, ./alloc +type FFIDestroyContentProc* = proc(content: pointer) {.nimcall, gcsafe.} + type FFIThreadRequest* = object callback: FFICallBack userData: pointer reqId*: cstring reqContent*: pointer + deleteReqContent*: FFIDestroyContentProc + ## Called by sendRequestToFFIThread on failure to free reqContent when + ## the FFI thread will never process (and thus never free) this request. proc init*( T: typedesc[FFIThreadRequest], @@ -26,7 +31,9 @@ proc init*( ret[].reqContent = reqContent return ret -proc deleteRequest(request: ptr FFIThreadRequest) = +proc deleteRequest*(request: ptr FFIThreadRequest) = + if not request[].deleteReqContent.isNil(): + request[].deleteReqContent(request[].reqContent) deallocShared(request[].reqId) deallocShared(request) @@ -48,9 +55,11 @@ proc handleRes*[T: string | void]( return foreignThreadGc: - var msg: cstring = "" + var resStr: string + ## we need to bind the string to extend its lifetime to callback's in ARC/ORC when T is string: - msg = res.get().cstring() + resStr = res.get() + let msg: cstring = resStr.cstring() request[].callback( RET_OK, unsafeAddr msg[0], cast[csize_t](len(msg)), request[].userData ) @@ -58,4 +67,3 @@ proc handleRes*[T: string | void]( proc nilProcess*(reqId: cstring): Future[Result[string, string]] {.async.} = return err("This request type is not implemented: " & $reqId) - diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 0aa7a86..5797045 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -164,6 +164,9 @@ proc buildFfiNewReqProc(reqTypeName, body: NimNode): NimNode = let typeStr = $T var ret = FFIThreadRequest.init(callback, userData, typeStr.cstring, `reqObjIdent`) + proc destroyContent(content: pointer) {.nimcall.} = + ffiDeleteReq(cast[ptr `reqTypeName`](content)) + ret[].deleteReqContent = destroyContent return ret ) @@ -255,8 +258,6 @@ proc buildProcessFFIRequestProc(reqTypeName, reqHandler, body: NimNode): NimNode newBody.add quote do: let `reqIdent`: ptr `reqTypeName` = cast[ptr `reqTypeName`](request) - defer: - ffiDeleteReq(`reqIdent`) # automatically unpack fields into locals for p in procParams[1 ..^ 1]: @@ -387,7 +388,7 @@ macro registerReqFFI*(reqTypeName, reqHandler, body: untyped): untyped = let processProc = buildProcessFFIRequestProc(reqTypeName, reqHandler, body) let addNewReqToReg = addNewRequestToRegistry(reqTypeName, reqHandler) let deleteProc = buildFfiDeleteReqProc(reqTypeName, fields) - result = newStmtList(typeDef, ffiNewReqProc, deleteProc, processProc, addNewReqToReg) + result = newStmtList(typeDef, deleteProc, ffiNewReqProc, processProc, addNewReqToReg) when defined(ffiDumpMacros): echo result.repr diff --git a/tests/test_alloc.nim b/tests/test_alloc.nim new file mode 100644 index 0000000..f07448c --- /dev/null +++ b/tests/test_alloc.nim @@ -0,0 +1,71 @@ +import unittest2 +import ../ffi/alloc + +suite "alloc(cstring)": + test "nil input returns empty cstring": + let s: cstring = nil + let res = alloc(s) + check res != nil + check res[0] == '\0' + deallocShared(res) + + test "copies content": + let res = alloc("hello world".cstring) + check $res == "hello world" + deallocShared(res) + + test "empty cstring": + let res = alloc("".cstring) + check len(res) == 0 + deallocShared(res) + +suite "alloc(string)": + test "copies content": + let res = alloc("test string") + check $res == "test string" + deallocShared(res) + + test "empty string": + let res = alloc("") + check len(res) == 0 + deallocShared(res) + + test "string with special characters": + let res = alloc("abc\0xyz") + check res[0] == 'a' + deallocShared(res) + +suite "allocSharedSeq / deallocSharedSeq / toSeq": + test "roundtrip int seq": + let original = @[1, 2, 3, 4, 5] + var shared = allocSharedSeq(original) + check shared.len == 5 + let back = toSeq(shared) + check back == original + deallocSharedSeq(shared) + check shared.len == 0 + + test "empty seq": + let original: seq[int] = @[] + var shared = allocSharedSeq(original) + check shared.len == 0 + check shared.data == nil + let back = toSeq(shared) + check back.len == 0 + deallocSharedSeq(shared) + + test "preserves element values by index": + let original = @[10, 20, 30] + var shared = allocSharedSeq(original) + check shared.data[0] == 10 + check shared.data[1] == 20 + check shared.data[2] == 30 + deallocSharedSeq(shared) + + test "works with byte seq": + let original = @[byte(0xFF), byte(0x00), byte(0xAB)] + var shared = allocSharedSeq(original) + check shared.len == 3 + let back = toSeq(shared) + check back == original + deallocSharedSeq(shared) diff --git a/tests/test_ffi_context.nim b/tests/test_ffi_context.nim new file mode 100644 index 0000000..9cb9542 --- /dev/null +++ b/tests/test_ffi_context.nim @@ -0,0 +1,137 @@ +import std/locks +import unittest2 +import results +import ../ffi + +type TestLib = object + +## Per-request callback state. The test thread blocks on `cond` until the +## FFI thread signals it — no polling, no CPU waste. +type CallbackData = object + lock: Lock + cond: Cond + called: bool + retCode: cint + msg: array[512, char] + msgLen: int + +proc initCallbackData(d: var CallbackData) = + d.lock.initLock() + d.cond.initCond() + +proc deinitCallbackData(d: var CallbackData) = + d.cond.deinitCond() + d.lock.deinitLock() + +proc testCallback( + retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer +) {.cdecl, gcsafe, raises: [].} = + let d = cast[ptr CallbackData](userData) + acquire(d[].lock) + d[].retCode = retCode + let n = min(int(len), d[].msg.high) + if n > 0 and not msg.isNil: + copyMem(addr d[].msg[0], msg, n) + d[].msg[n] = '\0' + d[].msgLen = n + d[].called = true + signal(d[].cond) + release(d[].lock) + +proc waitCallback(d: var CallbackData) = + acquire(d.lock) + while not d.called: + wait(d.cond, d.lock) + release(d.lock) + +proc callbackMsg(d: var CallbackData): string = + result = newString(d.msgLen) + if d.msgLen > 0: + copyMem(addr result[0], addr d.msg[0], d.msgLen) + +registerReqFFI(PingRequest, lib: ptr TestLib): + proc(message: cstring): Future[Result[string, string]] {.async.} = + return ok("pong:" & $message) + +registerReqFFI(FailRequest, lib: ptr TestLib): + proc(): Future[Result[string, string]] {.async.} = + return err("intentional failure") + +registerReqFFI(EmptyOkRequest, lib: ptr TestLib): + proc(): Future[Result[string, string]] {.async.} = + return ok("") + +suite "createFFIContext / destroyFFIContext": + test "create and destroy succeeds": + let ctx = createFFIContext[TestLib]().valueOr: + checkpoint "createFFIContext failed: " & $error + check false + return + check destroyFFIContext(ctx).isOk() + + test "double destroy is safe via running flag": + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + check destroyFFIContext(ctx).isOk() + +suite "sendRequestToFFIThread": + test "successful request triggers RET_OK callback": + var d: CallbackData + initCallbackData(d) + defer: deinitCallbackData(d) + + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, PingRequest.ffiNewReq(testCallback, addr d, "hello".cstring)).isOk() + waitCallback(d) + check d.retCode == RET_OK + check callbackMsg(d) == "pong:hello" + + test "failing request triggers RET_ERR callback": + var d: CallbackData + initCallbackData(d) + defer: deinitCallbackData(d) + + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, FailRequest.ffiNewReq(testCallback, addr d)).isOk() + waitCallback(d) + check d.retCode == RET_ERR + + test "empty ok response delivers empty message": + var d: CallbackData + initCallbackData(d) + defer: deinitCallbackData(d) + + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, EmptyOkRequest.ffiNewReq(testCallback, addr d)).isOk() + waitCallback(d) + check d.retCode == RET_OK + check d.msgLen == 0 + + test "sequential requests are all processed": + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + for i in 1 .. 5: + var d: CallbackData + initCallbackData(d) + let msg = "msg" & $i + check sendRequestToFFIThread(ctx, PingRequest.ffiNewReq(testCallback, addr d, msg.cstring)).isOk() + waitCallback(d) + deinitCallbackData(d) + check d.retCode == RET_OK + check callbackMsg(d) == "pong:" & msg