From 0305c1ace7f112dab6917bd6dc59177742b07688 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Mon, 4 May 2026 00:36:52 +0200 Subject: [PATCH] Fix cpp vulnerabilities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. No timeout → wait_for + 30 s default (ffi/codegen/cpp.nim) ffi_call_ now takes std::chrono::milliseconds timeout and uses cv.wait_for. All factory/method signatures carry a timeout parameter (default std::chrono::seconds{30}), mirroring the Rust blocking API. 2. Stack-allocated state → shared_ptr ownership (ffi/codegen/cpp.nim) ffi_cb_ now receives a heap-allocated std::shared_ptr* as user_data. The refcount is 2 going in (one for ffi_call_, one for the callback). If ffi_call_ times out and returns, its copy drops — but the state stays alive (refcount 1) until Nim eventually calls back and delete sptr in ffi_cb_ drops the last reference. No more stack UAF. 3. Destructor + Rule of 5 (ffi/codegen/cpp.nim, examples/nim_timer/nim_timer.nim) Added nimtimer_destroy to nim_timer.nim with {.dynlib, exportc, cdecl, raises: [].} — joins the FFI and watchdog threads, frees the context Codegen now always emits void {libName}_destroy(void* ctx) in extern "C" and generates a destructor, deleted copy ctor/assignment, and move ctor/assignment for the context class timeout_ stored in the class; move transfers it, destructor uses it 4. Hardcoded TimerConfig in createAsync (ffi/codegen/cpp.nim) createAsync now uses the actual ctorParams list (same as create), so it's correct for any library, not just nim_timer. 5. Opaque exceptions → clear error messages (ffi/codegen/cpp.nim) deserializeFfiResult wraps nlohmann::json::parse + .get() in a catch that rethrows as "FFI response deserialization failed: ...". The stoull in create() is also try-caught with "FFI create returned non-numeric address: " + raw. --- examples/nim_timer/cpp_bindings/nimtimer.hpp | 105 +++++++--- examples/nim_timer/nim_timer.nim | 8 + ffi/codegen/cpp.nim | 193 +++++++++++++------ 3 files changed, 221 insertions(+), 85 deletions(-) diff --git a/examples/nim_timer/cpp_bindings/nimtimer.hpp b/examples/nim_timer/cpp_bindings/nimtimer.hpp index e38f621..b753a4d 100644 --- a/examples/nim_timer/cpp_bindings/nimtimer.hpp +++ b/examples/nim_timer/cpp_bindings/nimtimer.hpp @@ -1,9 +1,11 @@ #pragma once #include #include +#include #include #include #include +#include #include #include #include @@ -71,9 +73,9 @@ int nimtimer_create(const char* config_json, FfiCallback callback, void* user_da int nimtimer_echo(void* ctx, FfiCallback callback, void* user_data, const char* req_json); int nimtimer_version(void* ctx, FfiCallback callback, void* user_data); int nimtimer_complex(void* ctx, FfiCallback callback, void* user_data, const char* req_json); +void nimtimer_destroy(void* ctx); } // extern "C" - template inline std::string serializeFfiArg(const T& value) { return nlohmann::json(value).dump(); @@ -85,12 +87,20 @@ inline std::string serializeFfiArg(void* value) { template inline T deserializeFfiResult(const std::string& raw) { - return nlohmann::json::parse(raw).get(); + try { + return nlohmann::json::parse(raw).get(); + } catch (const nlohmann::json::exception& e) { + throw std::runtime_error(std::string("FFI response deserialization failed: ") + e.what()); + } } template<> inline void* deserializeFfiResult(const std::string& raw) { - return reinterpret_cast(static_cast(std::stoull(raw))); + try { + return reinterpret_cast(static_cast(std::stoull(raw))); + } catch (const std::exception& e) { + throw std::runtime_error(std::string("FFI returned non-numeric address: ") + raw); + } } // ============================================================ @@ -108,24 +118,34 @@ struct FfiCallState_ { }; inline void ffi_cb_(int ret, const char* msg, size_t /*len*/, void* ud) { - auto* s = static_cast(ud); - std::lock_guard lock(s->mtx); - s->ok = (ret == 0); - s->msg = msg ? std::string(msg) : std::string{}; - s->done = true; - s->cv.notify_one(); + auto* sptr = static_cast*>(ud); + { + auto& s = **sptr; + std::lock_guard lock(s.mtx); + s.ok = (ret == 0); + s.msg = msg ? std::string(msg) : std::string{}; + s.done = true; + s.cv.notify_one(); + } + delete sptr; } -inline std::string ffi_call_(std::function f) { - FfiCallState_ state; - const int ret = f(ffi_cb_, &state); - if (ret == 2) +inline std::string ffi_call_(std::function f, + std::chrono::milliseconds timeout) { + auto state = std::make_shared(); + auto* cb_ref = new std::shared_ptr(state); + const int ret = f(ffi_cb_, cb_ref); + if (ret == 2) { + delete cb_ref; throw std::runtime_error("RET_MISSING_CALLBACK (internal error)"); - std::unique_lock lock(state.mtx); - state.cv.wait(lock, [&state]{ return state.done; }); - if (!state.ok) - throw std::runtime_error(state.msg); - return state.msg; + } + std::unique_lock lock(state->mtx); + const bool fired = state->cv.wait_for(lock, timeout, [&]{ return state->done; }); + if (!fired) + throw std::runtime_error("FFI call timed out after " + std::to_string(timeout.count()) + "ms"); + if (!state->ok) + throw std::runtime_error(state->msg); + return state->msg; } } // anonymous namespace @@ -136,25 +156,51 @@ inline std::string ffi_call_(std::function f) { class NimTimerCtx { public: - static NimTimerCtx create(const TimerConfig& config) { + static NimTimerCtx create(const TimerConfig& config, std::chrono::milliseconds timeout = std::chrono::seconds{30}) { const auto config_json = serializeFfiArg(config); const auto raw = ffi_call_([&](FfiCallback cb, void* ud) { return nimtimer_create(config_json.c_str(), cb, ud); - }); - // ctor returns the context address as a plain decimal string - const auto addr = std::stoull(raw); - return NimTimerCtx(reinterpret_cast(static_cast(addr))); + }, timeout); + try { + const auto addr = std::stoull(raw); + return NimTimerCtx(reinterpret_cast(static_cast(addr)), timeout); + } catch (const std::exception&) { + throw std::runtime_error("FFI create returned non-numeric address: " + raw); + } } - static std::future createAsync(const TimerConfig& config) { - return std::async(std::launch::async, [config]() { return create(config); }); + static std::future createAsync(const TimerConfig& config, std::chrono::milliseconds timeout = std::chrono::seconds{30}) { + return std::async(std::launch::async, [config, timeout]() { return create(config, timeout); }); + } + + ~NimTimerCtx() { + if (ptr_) { + nimtimer_destroy(ptr_); + ptr_ = nullptr; + } + } + + NimTimerCtx(const NimTimerCtx&) = delete; + NimTimerCtx& operator=(const NimTimerCtx&) = delete; + + NimTimerCtx(NimTimerCtx&& other) noexcept : ptr_(other.ptr_), timeout_(other.timeout_) { + other.ptr_ = nullptr; + } + NimTimerCtx& operator=(NimTimerCtx&& other) noexcept { + if (this != &other) { + if (ptr_) nimtimer_destroy(ptr_); + ptr_ = other.ptr_; + timeout_ = other.timeout_; + other.ptr_ = nullptr; + } + return *this; } EchoResponse echo(const EchoRequest& req) const { const auto req_json = serializeFfiArg(req); const auto raw = ffi_call_([&](FfiCallback cb, void* ud) { return nimtimer_echo(ptr_, cb, ud, req_json.c_str()); - }); + }, timeout_); return deserializeFfiResult(raw); } @@ -165,7 +211,7 @@ public: std::string version() const { const auto raw = ffi_call_([&](FfiCallback cb, void* ud) { return nimtimer_version(ptr_, cb, ud); - }); + }, timeout_); return deserializeFfiResult(raw); } @@ -177,7 +223,7 @@ public: const auto req_json = serializeFfiArg(req); const auto raw = ffi_call_([&](FfiCallback cb, void* ud) { return nimtimer_complex(ptr_, cb, ud, req_json.c_str()); - }); + }, timeout_); return deserializeFfiResult(raw); } @@ -187,5 +233,6 @@ public: private: void* ptr_; - explicit NimTimerCtx(void* p) : ptr_(p) {} + std::chrono::milliseconds timeout_; + explicit NimTimerCtx(void* p, std::chrono::milliseconds t) : ptr_(p), timeout_(t) {} }; diff --git a/examples/nim_timer/nim_timer.nim b/examples/nim_timer/nim_timer.nim index e595cf6..5e152ec 100644 --- a/examples/nim_timer/nim_timer.nim +++ b/examples/nim_timer/nim_timer.nim @@ -66,3 +66,11 @@ proc nimtimerComplex*( ok(ComplexResponse(summary: summary, itemCount: count, hasNote: req.note.isSome)) genBindings() # reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags + +proc nimtimer_destroy*(ctx: pointer) {.dynlib, exportc, cdecl, raises: [].} = + ## Tears down the FFI context created by nimtimer_create. + ## Blocks until the FFI thread and watchdog thread have joined. + try: + discard destroyFFIContext[NimTimer](cast[ptr FFIContext[NimTimer]](ctx)) + except: + discard diff --git a/ffi/codegen/cpp.nim b/ffi/codegen/cpp.nim index fea7dc0..91dd8b1 100644 --- a/ffi/codegen/cpp.nim +++ b/ffi/codegen/cpp.nim @@ -46,18 +46,23 @@ proc generateCppHeader*( ): string = var lines: seq[string] = @[] + # ── Includes ─────────────────────────────────────────────────────────────── lines.add("#pragma once") lines.add("#include ") lines.add("#include ") + lines.add("#include ") lines.add("#include ") lines.add("#include ") lines.add("#include ") + lines.add("#include ") lines.add("#include ") lines.add("#include ") lines.add("#include ") lines.add("#include ") lines.add("#include ") lines.add("") + + # ── nlohmann optional support ────────────────────────────────────────── lines.add("namespace nlohmann {") lines.add(" template") lines.add(" void to_json(json& j, const std::optional& opt) {") @@ -73,7 +78,7 @@ proc generateCppHeader*( lines.add("}") lines.add("") - # Types + # ── Types ────────────────────────────────────────────────────────────────── if types.len > 0: lines.add("// ============================================================") lines.add("// Types") @@ -82,8 +87,7 @@ proc generateCppHeader*( for t in types: lines.add("struct $1 {" % [t.name]) for f in t.fields: - let cppType = nimTypeToCpp(f.typeName) - lines.add(" $1 $2;" % [cppType, f.name]) + lines.add(" $1 $2;" % [nimTypeToCpp(f.typeName), f.name]) lines.add("};") var fieldNames: seq[string] = @[] for f in t.fields: @@ -93,7 +97,7 @@ proc generateCppHeader*( ) lines.add("") - # C extern declarations + # ── C FFI declarations ───────────────────────────────────────────────────── lines.add("// ============================================================") lines.add("// C FFI declarations") lines.add("// ============================================================") @@ -103,7 +107,6 @@ proc generateCppHeader*( "typedef void (*FfiCallback)(int ret, const char* msg, size_t len, void* user_data);" ) lines.add("") - for p in procs: var params: seq[string] = @[] if p.kind == ffiFfiKind: @@ -118,12 +121,12 @@ proc generateCppHeader*( params.add("FfiCallback callback") params.add("void* user_data") lines.add("int $1($2);" % [p.procName, params.join(", ")]) - + # Destroy is a plain synchronous call — no callback needed + lines.add("void $1_destroy(void* ctx);" % [libName]) lines.add("} // extern \"C\"") lines.add("") - # Transport serialization helpers - lines.add("") + # ── Serialization helpers ────────────────────────────────────────────────── lines.add("template") lines.add("inline std::string serializeFfiArg(const T& value) {") lines.add(" return nlohmann::json(value).dump();") @@ -133,20 +136,34 @@ proc generateCppHeader*( lines.add(" return std::to_string(reinterpret_cast(value));") lines.add("}") lines.add("") + # Wrap parse + get in a single try/catch so callers get a clear FFI error + # rather than a raw nlohmann exception with an opaque JSON pointer message. lines.add("template") lines.add("inline T deserializeFfiResult(const std::string& raw) {") - lines.add(" return nlohmann::json::parse(raw).get();") + lines.add(" try {") + lines.add(" return nlohmann::json::parse(raw).get();") + lines.add(" } catch (const nlohmann::json::exception& e) {") + lines.add( + " throw std::runtime_error(std::string(\"FFI response deserialization failed: \") + e.what());" + ) + lines.add(" }") lines.add("}") lines.add("") lines.add("template<>") lines.add("inline void* deserializeFfiResult(const std::string& raw) {") + lines.add(" try {") lines.add( - " return reinterpret_cast(static_cast(std::stoull(raw)));" + " return reinterpret_cast(static_cast(std::stoull(raw)));" ) + lines.add(" } catch (const std::exception& e) {") + lines.add( + " throw std::runtime_error(std::string(\"FFI returned non-numeric address: \") + raw);" + ) + lines.add(" }") lines.add("}") lines.add("") - # Anonymous namespace with synchronous call helper + # ── Call helper (anonymous namespace, header-only) ───────────────────────── lines.add("// ============================================================") lines.add("// Synchronous call helper (anonymous namespace, header-only)") lines.add("// ============================================================") @@ -161,46 +178,63 @@ proc generateCppHeader*( lines.add(" std::string msg;") lines.add("};") lines.add("") + # user_data is a heap-allocated shared_ptr. + # Ownership: ffi_call_ holds one copy; this callback holds the other. + # When ffi_call_ times out and returns before the callback fires, the + # state stays alive (refcount 1) until Nim eventually calls back and + # deletes cb_ref — eliminating the UAF that a stack-allocated state has. lines.add("inline void ffi_cb_(int ret, const char* msg, size_t /*len*/, void* ud) {") - lines.add(" auto* s = static_cast(ud);") - lines.add(" std::lock_guard lock(s->mtx);") - lines.add(" s->ok = (ret == 0);") - lines.add(" s->msg = msg ? std::string(msg) : std::string{};") - lines.add(" s->done = true;") - lines.add(" s->cv.notify_one();") + lines.add(" auto* sptr = static_cast*>(ud);") + lines.add(" {") + lines.add(" auto& s = **sptr;") + lines.add(" std::lock_guard lock(s.mtx);") + lines.add(" s.ok = (ret == 0);") + lines.add(" s.msg = msg ? std::string(msg) : std::string{};") + lines.add(" s.done = true;") + lines.add(" s.cv.notify_one();") + lines.add(" }") + lines.add(" delete sptr;") lines.add("}") lines.add("") - lines.add("inline std::string ffi_call_(std::function f) {") - lines.add(" FfiCallState_ state;") - lines.add(" const int ret = f(ffi_cb_, &state);") - lines.add(" if (ret == 2)") + lines.add( + "inline std::string ffi_call_(std::function f," + ) + lines.add(" std::chrono::milliseconds timeout) {") + lines.add(" auto state = std::make_shared();") + lines.add(" auto* cb_ref = new std::shared_ptr(state);") + lines.add(" const int ret = f(ffi_cb_, cb_ref);") + lines.add(" if (ret == 2) {") + lines.add(" delete cb_ref;") lines.add( " throw std::runtime_error(\"RET_MISSING_CALLBACK (internal error)\");" ) - lines.add(" std::unique_lock lock(state.mtx);") - lines.add(" state.cv.wait(lock, [&state]{ return state.done; });") - lines.add(" if (!state.ok)") - lines.add(" throw std::runtime_error(state.msg);") - lines.add(" return state.msg;") + lines.add(" }") + lines.add(" std::unique_lock lock(state->mtx);") + lines.add( + " const bool fired = state->cv.wait_for(lock, timeout, [&]{ return state->done; });" + ) + lines.add(" if (!fired)") + lines.add( + " throw std::runtime_error(\"FFI call timed out after \" + std::to_string(timeout.count()) + \"ms\");" + ) + lines.add(" if (!state->ok)") + lines.add(" throw std::runtime_error(state->msg);") + lines.add(" return state->msg;") lines.add("}") lines.add("") lines.add("} // anonymous namespace") lines.add("") - # Derive context type name and separate ctors / methods + # ── High-level C++ context class ────────────────────────────────────────── var ctors: seq[FFIProcMeta] = @[] var methods: seq[FFIProcMeta] = @[] for p in procs: - if p.kind == ffiCtorKind: - ctors.add(p) - else: - methods.add(p) + if p.kind == ffiCtorKind: ctors.add(p) + else: methods.add(p) let libTypeName = - if ctors.len > 0: - ctors[0].libTypeName - else: - libName[0 .. 0].toUpperAscii() & libName[1 .. ^1] + if ctors.len > 0: ctors[0].libTypeName + else: libName[0 .. 0].toUpperAscii() & libName[1 .. ^1] let ctxTypeName = libTypeName & "Ctx" @@ -211,45 +245,92 @@ proc generateCppHeader*( lines.add("class $1 {" % [ctxTypeName]) lines.add("public:") - # Static create() factory + # ── Constructors ──────────────────────────────────────────────────────── for ctor in ctors: var ctorParams: seq[string] = @[] + var epNames: seq[string] = @[] for ep in ctor.extraParams: - let cppType = nimTypeToCpp(ep.typeName) - ctorParams.add("const $1& $2" % [cppType, ep.name]) + ctorParams.add("const $1& $2" % [nimTypeToCpp(ep.typeName), ep.name]) + epNames.add(ep.name) + let timeoutParam = "std::chrono::milliseconds timeout = std::chrono::seconds{30}" + let ctorParamsWithTimeout = + if ctorParams.len > 0: ctorParams.join(", ") & ", " & timeoutParam + else: timeoutParam - lines.add(" static $1 create($2) {" % [ctxTypeName, ctorParams.join(", ")]) + # -- create() factory -- + lines.add(" static $1 create($2) {" % [ctxTypeName, ctorParamsWithTimeout]) for ep in ctor.extraParams: lines.add(" const auto $1_json = serializeFfiArg($1);" % [ep.name]) - var callArgs: seq[string] = @[] for ep in ctor.extraParams: callArgs.add("$1_json.c_str()" % [ep.name]) callArgs.add("cb") callArgs.add("ud") - lines.add(" const auto raw = ffi_call_([&](FfiCallback cb, void* ud) {") lines.add(" return $1($2);" % [ctor.procName, callArgs.join(", ")]) - lines.add(" });") - lines.add(" // ctor returns the context address as a plain decimal string") - lines.add(" const auto addr = std::stoull(raw);") + lines.add(" }, timeout);") + lines.add(" try {") + lines.add(" const auto addr = std::stoull(raw);") lines.add( - " return $1(reinterpret_cast(static_cast(addr)));" % + " return $1(reinterpret_cast(static_cast(addr)), timeout);" % [ctxTypeName] ) + lines.add(" } catch (const std::exception&) {") + lines.add( + " throw std::runtime_error(\"FFI create returned non-numeric address: \" + raw);" + ) + lines.add(" }") lines.add(" }") lines.add("") + + # -- createAsync() factory: uses actual param types, not hardcoded -- + let captureList = + if epNames.len > 0: epNames.join(", ") & ", timeout" + else: "timeout" + let callList = + if epNames.len > 0: epNames.join(", ") & ", timeout" + else: "timeout" lines.add( - " static std::future<$1> createAsync(const TimerConfig& config) {" % - [ctxTypeName] + " static std::future<$1> createAsync($2) {" % + [ctxTypeName, ctorParamsWithTimeout] ) lines.add( - " return std::async(std::launch::async, [config]() { return create(config); });" + " return std::async(std::launch::async, [$1]() { return create($2); });" % + [captureList, callList] ) lines.add(" }") lines.add("") - # Instance methods + # ── Rule of 5 ────────────────────────────────────────────────────────── + # Destructor tears down Nim threads; copies are deleted; moves transfer ownership. + lines.add(" ~$1() {" % [ctxTypeName]) + lines.add(" if (ptr_) {") + lines.add(" $1_destroy(ptr_);" % [libName]) + lines.add(" ptr_ = nullptr;") + lines.add(" }") + lines.add(" }") + lines.add("") + lines.add(" $1(const $1&) = delete;" % [ctxTypeName]) + lines.add(" $1& operator=(const $1&) = delete;" % [ctxTypeName]) + lines.add("") + lines.add( + " $1($1&& other) noexcept : ptr_(other.ptr_), timeout_(other.timeout_) {" % + [ctxTypeName] + ) + lines.add(" other.ptr_ = nullptr;") + lines.add(" }") + lines.add(" $1& operator=($1&& other) noexcept {" % [ctxTypeName]) + lines.add(" if (this != &other) {") + lines.add(" if (ptr_) $1_destroy(ptr_);" % [libName]) + lines.add(" ptr_ = other.ptr_;") + lines.add(" timeout_ = other.timeout_;") + lines.add(" other.ptr_ = nullptr;") + lines.add(" }") + lines.add(" return *this;") + lines.add(" }") + lines.add("") + + # ── Instance methods ──────────────────────────────────────────────────── for m in methods: let methodName = stripLibPrefixCpp(m.procName, libName) let retCppType = nimTypeToCpp(m.returnTypeName) @@ -257,8 +338,7 @@ proc generateCppHeader*( var methParams: seq[string] = @[] var methParamNames: seq[string] = @[] for ep in m.extraParams: - let cppType = nimTypeToCpp(ep.typeName) - methParams.add("const $1& $2" % [cppType, ep.name]) + methParams.add("const $1& $2" % [nimTypeToCpp(ep.typeName), ep.name]) methParamNames.add(ep.name) let methParamsStr = methParams.join(", ") let methParamNamesStr = methParamNames.join(", ") @@ -266,15 +346,12 @@ proc generateCppHeader*( lines.add(" $1 $2($3) const {" % [retCppType, methodName, methParamsStr]) for ep in m.extraParams: lines.add(" const auto $1_json = serializeFfiArg($1);" % [ep.name]) - var callArgs = @["ptr_", "cb", "ud"] for ep in m.extraParams: callArgs.add("$1_json.c_str()" % [ep.name]) - lines.add(" const auto raw = ffi_call_([&](FfiCallback cb, void* ud) {") lines.add(" return $1($2);" % [m.procName, callArgs.join(", ")]) - lines.add(" });") - + lines.add(" }, timeout_);") if retCppType == "void*": lines.add(" return deserializeFfiResult(raw);") else: @@ -302,7 +379,11 @@ proc generateCppHeader*( lines.add("private:") lines.add(" void* ptr_;") - lines.add(" explicit $1(void* p) : ptr_(p) {}" % [ctxTypeName]) + lines.add(" std::chrono::milliseconds timeout_;") + lines.add( + " explicit $1(void* p, std::chrono::milliseconds t) : ptr_(p), timeout_(t) {}" % + [ctxTypeName] + ) lines.add("};") lines.add("")