From 81c62c263e26775bb38a5ae884d23eed9d1ea6b4 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com> Date: Mon, 11 May 2026 19:21:40 -0300 Subject: [PATCH] fix: context buffer overflow (#21) --- .gitignore | 4 ++++ ffi.nimble | 1 + ffi/ffi_context.nim | 37 ++++++++++++++++++++++++++++--- ffi/internal/ffi_macro.nim | 5 +++-- tests/test_ctx_validation.nim | 41 +++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 tests/test_ctx_validation.nim diff --git a/.gitignore b/.gitignore index 0d37f33..476ac9d 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,7 @@ examples/**/rust_client/target/ # Development plan (local only) PLAN.md + +# Compiled test binaries (extensionless executables) +tests/test_* +!tests/test_*.nim diff --git a/ffi.nimble b/ffi.nimble index 8dc08f4..1694abd 100644 --- a/ffi.nimble +++ b/ffi.nimble @@ -24,6 +24,7 @@ task test, "Run all tests under --mm:orc and --mm:refc": exec "nim c -r " & flags & " tests/test_ffi_context.nim" exec "nim c -r " & flags & " tests/test_gc_compat.nim" exec "nim c -r " & flags & " tests/test_serial.nim" + exec "nim c -r " & flags & " tests/test_ctx_validation.nim" task test_alloc, "Run alloc unit tests under --mm:orc and --mm:refc": exec "nim c -r " & nimFlagsOrc & " tests/test_alloc.nim" diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index dd9b705..48d33e4 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -2,7 +2,7 @@ {.pragma: callback, cdecl, raises: [], gcsafe.} {.passc: "-fPIC".} -import std/[options, atomics, os, net, locks, json, tables] +import std/[options, atomics, os, net, locks, json, tables, sets] import chronicles, chronos, chronos/threadsync, taskpools/channels_spsc_single, results import ./ffi_types, ./ffi_thread_request, ./internal/ffi_macro, ./logging @@ -41,6 +41,30 @@ var ffiCurrentCallbackState* {.threadvar.}: ptr FFICallbackState const git_version* {.strdefine.} = "n/a" +var contextRegistry = initHashSet[pointer]() +var contextRegistryLock: Lock +contextRegistryLock.initLock() + +proc registerCtx(ctx: pointer) = + {.cast(gcsafe).}: + contextRegistryLock.acquire() + defer: contextRegistryLock.release() + contextRegistry.incl(ctx) + +proc unregisterCtx(ctx: pointer) = + {.cast(gcsafe).}: + contextRegistryLock.acquire() + defer: contextRegistryLock.release() + contextRegistry.excl(ctx) + +proc isValidCtx*(ctx: pointer): bool = + ## Returns true only if ctx was created by createFFIContext and not yet destroyed. + ## Rejects nil, offset-invalid, and dangling pointers at the API boundary. + {.cast(gcsafe).}: + contextRegistryLock.acquire() + defer: contextRegistryLock.release() + return contextRegistry.contains(ctx) + template callEventCallback*(ctx: ptr FFIContext, eventName: string, body: untyped) = if isNil(ctx[].callbackState.callback): chronicles.error eventName & " - eventCallback is nil" @@ -84,6 +108,9 @@ template dispatchFfiEvent*(eventName: string, body: untyped) = proc sendRequestToFFIThread*( ctx: ptr FFIContext, ffiRequest: ptr FFIThreadRequest, timeout = InfiniteDuration ): Result[void, string] = + if not isValidCtx(cast[pointer](ctx)): + deleteRequest(ffiRequest) + return err("ctx is not a valid FFI context") ctx.lock.acquire() # This lock is only necessary while we use a SP Channel and while the signalling # between threads assumes that there aren't concurrent requests. @@ -336,6 +363,7 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = joinThread(ctx.ffiThread) return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) + registerCtx(cast[pointer](ctx)) success = true return ok(ctx) @@ -346,6 +374,7 @@ proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## the thread will eventually exit on its own, but cleanup is skipped ## because the thread may still be touching ctx fields. const ThreadExitTimeout = 1500.milliseconds + unregisterCtx(cast[pointer](ctx)) ctx.running.store(false) @@ -380,8 +409,10 @@ proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = return ok() template checkParams*(ctx: ptr FFIContext, callback: FFICallBack, userData: pointer) = - if not isNil(ctx): - ctx[].userData = userData + if not isValidCtx(cast[pointer](ctx)): + return RET_ERR + + ctx[].userData = userData if isNil(callback): return RET_MISSING_CALLBACK diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 9a75f93..46642b0 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -569,8 +569,9 @@ macro ffiRaw*(prc: untyped): untyped = let ffiBody = newStmtList( quote do: initializeLibrary() - if not isNil(ctx): - ctx[].userData = userData + if not isValidCtx(cast[pointer](ctx)): + return RET_ERR + ctx[].userData = userData if isNil(callback): return RET_MISSING_CALLBACK ) diff --git a/tests/test_ctx_validation.nim b/tests/test_ctx_validation.nim new file mode 100644 index 0000000..07b2547 --- /dev/null +++ b/tests/test_ctx_validation.nim @@ -0,0 +1,41 @@ +import std/locks +import unittest2 +import results +import ../ffi + +type TestLib = object + +proc dummyCallback( + retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer +) {.cdecl, gcsafe, raises: [].} = + discard + +registerReqFFI(ValidationTestRequest, lib: ptr TestLib): + proc(): Future[Result[string, string]] {.async.} = + return ok("ok") + +suite "ctx pointer validation": + # BUG: sendRequestToFFIThread has no nil-check on ctx. + # checkParams / {.ffi.} generated code only guards against nil callback, + # not nil (or otherwise invalid) ctx. Any caller — C or Nim — that passes + # a nil or offset-invalid ctx with a valid callback bypasses the only guard + # and reaches ctx.lock.acquire() where the nil/garbage dereference crashes. + + test "nil ctx with valid callback should return an error, not crash": + # Reproduces the nil case: ctx=nil, callback=valid. + # Expected (after fix): sendRequestToFFIThread returns isErr(). + # Actual (currently) : SIGSEGV at ctx.lock.acquire() in sendRequestToFFIThread. + let nilCtx: ptr FFIContext[TestLib] = nil + let req = ValidationTestRequest.ffiNewReq(dummyCallback, nil) + let res = sendRequestToFFIThread(nilCtx, req) + check res.isErr() + + test "invalid non-nil ctx (ctx+123 style) should return an error, not crash": + # Reproduces the offset-pointer case: a non-nil but invalid pointer passes + # isNil() and reaches the lock dereference, causing a crash. + # Expected (after fix): sendRequestToFFIThread returns isErr(). + # Actual (currently) : SIGSEGV when the garbage pointer is dereferenced. + let invalidCtx = cast[ptr FFIContext[TestLib]](123) + let req = ValidationTestRequest.ffiNewReq(dummyCallback, nil) + let res = sendRequestToFFIThread(invalidCtx, req) + check res.isErr()