From 974d06d77523c3e2f7c7e31a132263f895d352da Mon Sep 17 00:00:00 2001 From: osmaczko <33099791+osmaczko@users.noreply.github.com> Date: Wed, 25 Feb 2026 21:24:39 +0100 Subject: [PATCH] fix(nim-bindings): add all-endpoints test and fix installation_name ABI Add nim-bindings/tests/test_all_endpoints.nim which imports bindings directly and calls every FFI proc, forcing the linker to include all symbols. This catches link-time and runtime issues that the pingpong example missed because unused symbols were optimised out. Running the new test revealed an ABI mismatch in installation_name: the Rust function used an explicit out-parameter but ReprCString has only flat fields, so Nim emits it as a C return value. CI now runs nimble test next to nimble pingpong. --- .github/workflows/ci.yml | 2 + Cargo.toml | 5 + conversations/Cargo.toml | 2 +- conversations/src/api.rs | 17 +- ...sations_example.nimble => bindings.nimble} | 4 + nim-bindings/src/bindings.nim | 5 + nim-bindings/src/libchat.nim | 1 + nim-bindings/tests/test_all_endpoints.nim | 258 ++++++++++++++++++ 8 files changed, 289 insertions(+), 5 deletions(-) rename nim-bindings/{conversations_example.nimble => bindings.nimble} (75%) create mode 100644 nim-bindings/tests/test_all_endpoints.nim diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cabce2f..5cbd2ad 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,5 +52,7 @@ jobs: echo "$HOME/.nimble/bin" >> $GITHUB_PATH - run: nimble install -dy working-directory: nim-bindings + - run: nimble test + working-directory: nim-bindings - run: nimble pingpong working-directory: nim-bindings diff --git a/Cargo.toml b/Cargo.toml index 0381c7d..274eb12 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,3 +12,8 @@ members = [ [workspace.dependencies] blake2 = "0.10" storage = { path = "storage" } + +# Panicking across FFI boundaries is UB; abort is the correct strategy for a +# C FFI library. +[profile.release] +panic = "abort" diff --git a/conversations/Cargo.toml b/conversations/Cargo.toml index 6503920..cdb02a9 100644 --- a/conversations/Cargo.toml +++ b/conversations/Cargo.toml @@ -4,7 +4,7 @@ version = "0.1.0" edition = "2024" [lib] -crate-type = ["staticlib","dylib"] +crate-type = ["rlib","staticlib","dylib"] [dependencies] base64 = "0.22" diff --git a/conversations/src/api.rs b/conversations/src/api.rs index 67cb483..bd1e300 100644 --- a/conversations/src/api.rs +++ b/conversations/src/api.rs @@ -56,11 +56,9 @@ pub fn create_context(name: repr_c::String) -> repr_c::Box { /// Returns the friendly name of the contexts installation. /// -/// # ABI note -/// The result is written through `out` (Nim's calling convention for large struct returns). #[ffi_export] -pub fn installation_name(ctx: &ContextHandle, out: &mut repr_c::String) { - *out = ctx.0.installation_name().to_string().into(); +pub fn installation_name(ctx: &ContextHandle) -> repr_c::String { + ctx.0.installation_name().to_string().into() } /// Destroys a conversation store and frees its memory @@ -74,6 +72,17 @@ pub fn destroy_context(ctx: repr_c::Box) { drop(ctx); } +/// Destroys a repr_c::String and frees its memory +/// +/// # Safety +/// - s must be an owned repr_c::String value returned from a libchat FFI function +/// - s must not be used after this call +/// - s must not be freed twice +#[ffi_export] +pub fn destroy_string(s: repr_c::String) { + drop(s); +} + /// Creates an intro bundle for sharing with other users /// /// # Returns diff --git a/nim-bindings/conversations_example.nimble b/nim-bindings/bindings.nimble similarity index 75% rename from nim-bindings/conversations_example.nimble rename to nim-bindings/bindings.nimble index 236a45f..b20ee5f 100644 --- a/nim-bindings/conversations_example.nimble +++ b/nim-bindings/bindings.nimble @@ -24,3 +24,7 @@ before build: task pingpong, "Run pingpong example": buildRust() exec "nim c -r --path:src --passL:../target/release/liblibchat.a --passL:-lm examples/pingpong.nim" + +task test, "Run comprehensive all-endpoints test": + buildRust() + exec "nim c -r --path:src --passL:../target/release/liblibchat.a --passL:-lm tests/test_all_endpoints.nim" diff --git a/nim-bindings/src/bindings.nim b/nim-bindings/src/bindings.nim index 81b7505..eb6053e 100644 --- a/nim-bindings/src/bindings.nim +++ b/nim-bindings/src/bindings.nim @@ -96,6 +96,11 @@ proc installation_name*(ctx: ContextHandle): ReprCString {.importc.} ## - handle must not be used after this call proc destroy_context*(ctx: ContextHandle) {.importc.} +## Free a ReprCString returned by any of the FFI functions +## - s must be an owned ReprCString value returned from an FFI function +## - s must not be used after this call +proc destroy_string*(s: ReprCString) {.importc.} + ## Creates an intro bundle for sharing with other users ## Returns: CreateIntroResult struct - check error_code field (0 = success, negative = error) ## The result must be freed with destroy_intro_result() diff --git a/nim-bindings/src/libchat.nim b/nim-bindings/src/libchat.nim index 0e2494a..90960c1 100644 --- a/nim-bindings/src/libchat.nim +++ b/nim-bindings/src/libchat.nim @@ -25,6 +25,7 @@ proc getInstallationName*(ctx: LibChat): string = if ctx.handle == nil: return "" let name = installation_name(ctx.handle) + defer: destroy_string(name) result = $name ## Destroy the context and free resources diff --git a/nim-bindings/tests/test_all_endpoints.nim b/nim-bindings/tests/test_all_endpoints.nim new file mode 100644 index 0000000..e9e3f54 --- /dev/null +++ b/nim-bindings/tests/test_all_endpoints.nim @@ -0,0 +1,258 @@ +# Comprehensive test for all FFI procs declared in bindings.nim. +# +# Design intent: By importing `bindings` directly and calling every importc +# proc at least once, the linker is forced to include ALL symbol references. +# This prevents link-time optimizations from stripping unused symbols and +# catches both link-time crashes (missing symbols) and runtime crashes +# (wrong ABI, segfaults on use). + +import bindings + +# --------------------------------------------------------------------------- +# Assertion helper +# --------------------------------------------------------------------------- + +proc check(cond: bool, msg: string) = + if not cond: + echo "FAIL: ", msg + quit(1) + echo "OK: ", msg + +# --------------------------------------------------------------------------- +# Section 1: Helper proc coverage +# --------------------------------------------------------------------------- + +proc testHelperProcs() = + echo "\n--- testHelperProcs ---" + + # toSlice(string) — non-empty and empty branches + let s = "hello" + let sl = toSlice(s) + check(sl.len == 5, "toSlice(string): correct len") + check(sl.`ptr` != nil, "toSlice(non-empty string): non-nil ptr") + + let emptySl = toSlice("") + check(emptySl.len == 0, "toSlice(empty string): len == 0") + check(emptySl.`ptr` == nil, "toSlice(empty string): ptr == nil") + + # toSlice(seq[byte]) — non-empty and empty branches + let b: seq[byte] = @[0x61'u8, 0x62'u8, 0x63'u8] + let bSl = toSlice(b) + check(bSl.len == 3, "toSlice(seq[byte]): correct len") + check(bSl.`ptr` != nil, "toSlice(non-empty seq[byte]): non-nil ptr") + + let emptyBSl = toSlice(newSeq[byte](0)) + check(emptyBSl.len == 0, "toSlice(empty seq[byte]): len == 0") + check(emptyBSl.`ptr` == nil, "toSlice(empty seq[byte]): ptr == nil") + + # toReprCString(string) and $(ReprCString) round-trip + let name = "testname" + let rcs = toReprCString(name) + check(rcs.len == csize_t(name.len), "toReprCString: correct len") + check(rcs.cap == 0, "toReprCString: cap == 0 (prevents Rust dealloc of Nim memory)") + check(rcs.`ptr` != nil, "toReprCString: non-nil ptr") + check($rcs == name, "$(ReprCString): round-trips to original string") + + let emptyRcs = toReprCString("") + check(emptyRcs.len == 0, "toReprCString(empty): len == 0") + check($emptyRcs == "", "$(empty ReprCString): returns empty string") + + # toBytes(string) + let bs = toBytes("abc") + check(bs.len == 3, "toBytes: correct length") + check(bs[0] == 0x61'u8, "toBytes: correct first byte") + + let emptyBs = toBytes("") + check(emptyBs.len == 0, "toBytes(empty): empty seq") + +# --------------------------------------------------------------------------- +# Section 2: create_context / installation_name / destroy_context +# --------------------------------------------------------------------------- + +proc testContextLifecycle() = + echo "\n--- testContextLifecycle ---" + + let ctx = create_context(toReprCString("lifecycle-test")) + check(ctx != nil, "create_context: returns non-nil handle") + + let iname = installation_name(ctx) + defer: destroy_string(iname) + let inameStr = $iname + check(inameStr.len > 0, "installation_name: returns non-empty name") + echo " installation name: ", inameStr + + destroy_context(ctx) + echo " destroy_context: no crash" + +# --------------------------------------------------------------------------- +# Section 3: Full two-party conversation flow +# --------------------------------------------------------------------------- +# Exercises: create_intro_bundle, create_new_private_convo, handle_payload, +# send_content, and all four destroy_* procs. +# VecPayload helpers ([], len, items) are also exercised here. + +proc testFullConversationFlow() = + echo "\n--- testFullConversationFlow ---" + + let aliceCtx = create_context(toReprCString("alice")) + check(aliceCtx != nil, "Alice: create_context non-nil") + + let bobCtx = create_context(toReprCString("bob")) + check(bobCtx != nil, "Bob: create_context non-nil") + + # --- create_intro_bundle --- + var bobIntroRes = create_intro_bundle(bobCtx) + check(bobIntroRes.error_code == ErrNone, + "create_intro_bundle: error_code == ErrNone") + check(bobIntroRes.intro_bytes.len > 0, + "create_intro_bundle: intro_bytes non-empty") + + # toSeq(VecUint8) + let introBytes = toSeq(bobIntroRes.intro_bytes) + check(introBytes.len > 0, "toSeq(VecUint8): produces non-empty seq") + + # destroy_intro_result + destroy_intro_result(bobIntroRes) + echo " destroy_intro_result: no crash" + + # --- create_new_private_convo --- + var convoRes = create_new_private_convo( + aliceCtx, + toSlice(introBytes), + toSlice("Hello, Bob!") + ) + check(convoRes.error_code == ErrNone, + "create_new_private_convo: error_code == ErrNone") + + let aliceConvoId = $convoRes.convo_id + check(aliceConvoId.len > 0, "create_new_private_convo: convo_id non-empty") + echo " Alice-Bob convo_id: ", aliceConvoId + + # len(VecPayload) + let numPayloads = len(convoRes.payloads) + check(numPayloads > 0, "len(VecPayload): > 0 payloads in new convo") + + # [](VecPayload, int): subscript access + let firstPayload = convoRes.payloads[0] + check(firstPayload.data.len > 0, "VecPayload[0].data: non-empty") + check(firstPayload.address.len > 0, "VecPayload[0].address: non-empty") + echo " first payload address: ", $firstPayload.address + + # items(VecPayload): collect bytes before destroy + var payloadDatas: seq[seq[byte]] = @[] + var iterCount = 0 + for p in convoRes.payloads: + payloadDatas.add(toSeq(p.data)) + inc iterCount + check(iterCount == numPayloads, + "items(VecPayload): iterator yields all payloads") + + # destroy_convo_result + destroy_convo_result(convoRes) + echo " destroy_convo_result: no crash" + + # --- handle_payload --- + var bobSawContent = false + var bobConvoId = "" + for pData in payloadDatas: + var hp = handle_payload(bobCtx, toSlice(pData)) + check(hp.error_code == ErrNone, "handle_payload: error_code == ErrNone") + + let content = toSeq(hp.content) + if content.len > 0: + bobConvoId = $hp.convo_id + check(bobConvoId.len > 0, + "handle_payload: convo_id non-empty when content present") + if not bobSawContent: + check(hp.is_new_convo, + "handle_payload: is_new_convo == true on first contact") + bobSawContent = true + echo " Bob received content in convo: ", bobConvoId + + destroy_handle_payload_result(hp) + + check(bobSawContent, "handle_payload: Bob received Alice's opening message") + echo " destroy_handle_payload_result: no crash" + + # --- send_content --- + var sendRes = send_content( + aliceCtx, + toReprCString(aliceConvoId), + toSlice("How are you, Bob?") + ) + check(sendRes.error_code == ErrNone, + "send_content: error_code == ErrNone for valid convo_id") + check(len(sendRes.payloads) > 0, + "send_content: returns at least one payload") + + var sendPayloadDatas: seq[seq[byte]] = @[] + for p in sendRes.payloads: + sendPayloadDatas.add(toSeq(p.data)) + + # destroy_send_content_result + destroy_send_content_result(sendRes) + echo " destroy_send_content_result: no crash" + + # Bob handles follow-up payloads + for pData in sendPayloadDatas: + var hp2 = handle_payload(bobCtx, toSlice(pData)) + check(hp2.error_code == ErrNone, + "handle_payload: Bob handles send_content payload without error") + destroy_handle_payload_result(hp2) + + destroy_context(aliceCtx) + destroy_context(bobCtx) + echo " both contexts destroyed: no crash" + +# --------------------------------------------------------------------------- +# Section 4: Error-case coverage +# --------------------------------------------------------------------------- +# Exercises destroy_* on error results (empty/null Vecs) to confirm they +# do not crash. + +proc testErrorCases() = + echo "\n--- testErrorCases ---" + + let ctx = create_context(toReprCString("error-tester")) + check(ctx != nil, "error-tester: create_context non-nil") + + # send_content with a nonexistent convo_id must fail + var badSend = send_content( + ctx, + toReprCString("00000000-0000-0000-0000-nonexistent"), + toSlice("payload") + ) + check(badSend.error_code != ErrNone, + "send_content(bad convo_id): error_code != ErrNone") + echo " send_content(bad convo_id) error_code: ", badSend.error_code + # Destroy error result to confirm destroy handles empty VecPayload + destroy_send_content_result(badSend) + echo " destroy_send_content_result(error result): no crash" + + # create_new_private_convo with garbage bytes must fail with ErrBadIntro + let badIntro: seq[byte] = @[0xDE'u8, 0xAD'u8, 0xBE'u8, 0xEF'u8] + var badConvo = create_new_private_convo( + ctx, + toSlice(badIntro), + toSlice("content") + ) + check(badConvo.error_code == ErrBadIntro, + "create_new_private_convo(bad intro): error_code == ErrBadIntro") + destroy_convo_result(badConvo) + echo " destroy_convo_result(error result): no crash" + + destroy_context(ctx) + +# --------------------------------------------------------------------------- +# Entry point +# --------------------------------------------------------------------------- + +when isMainModule: + echo "=== test_all_endpoints: begin ===" + + testHelperProcs() + testContextLifecycle() + testFullConversationFlow() + testErrorCases() + + echo "\n=== ALL TESTS PASSED ==="