From 1f8c38d2ce5868bb2b4c4878afcd10627d889da7 Mon Sep 17 00:00:00 2001 From: osmaczko <33099791+osmaczko@users.noreply.github.com> Date: Wed, 25 Feb 2026 17:21:49 +0100 Subject: [PATCH] fix(nim-bindings): fix ABI mismatch in destroy_* FFI functions and add defer-based cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nim's C backend silently transforms large struct parameters (>16 bytes) into pointer parameters when calling importc functions. The destroy_* functions were declared taking T by value in Rust, but Nim always passed &T — causing Rust to read garbage from the stack on x86-64 (SIGILL on CI) while accidentally working on ARM64 macOS due to that ABI coincidentally also using pointers for large structs. Fix by changing all destroy_* functions to take &mut T and using drop_in_place, which is the correct idiom for dropping a value through a pointer. On the Nim side, replace scattered manual destroy calls with defer, which guarantees cleanup on all exit paths and prevents use-after-destroy bugs. --- conversations/src/api.rs | 32 +++++++++++++++----- nim-bindings/src/libchat.nim | 58 ++++++++---------------------------- 2 files changed, 37 insertions(+), 53 deletions(-) diff --git a/conversations/src/api.rs b/conversations/src/api.rs index 31fb9d9..6689541 100644 --- a/conversations/src/api.rs +++ b/conversations/src/api.rs @@ -214,9 +214,13 @@ pub struct CreateIntroResult { } /// Free the result from create_intro_bundle +/// +/// # ABI note +/// Takes `&mut` instead of ownership because Nim always passes large structs as a pointer; +/// accepting the struct by value would be an ABI mismatch on the caller side. #[ffi_export] -pub fn destroy_intro_result(result: CreateIntroResult) { - drop(result); +pub fn destroy_intro_result(result: &mut CreateIntroResult) { + unsafe { std::ptr::drop_in_place(result) } } /// Payload structure for FFI @@ -238,9 +242,13 @@ pub struct SendContentResult { } /// Free the result from send_content +/// +/// # ABI note +/// Takes `&mut` instead of ownership because Nim always passes large structs as a pointer; +/// accepting the struct by value would be an ABI mismatch on the caller side. #[ffi_export] -pub fn destroy_send_content_result(result: SendContentResult) { - drop(result); +pub fn destroy_send_content_result(result: &mut SendContentResult) { + unsafe { std::ptr::drop_in_place(result) } } /// Result structure for handle_payload @@ -256,9 +264,13 @@ pub struct HandlePayloadResult { } /// Free the result from handle_payload +/// +/// # ABI note +/// Takes `&mut` instead of ownership because Nim always passes large structs as a pointer; +/// accepting the struct by value would be an ABI mismatch on the caller side. #[ffi_export] -pub fn destroy_handle_payload_result(result: HandlePayloadResult) { - drop(result); +pub fn destroy_handle_payload_result(result: &mut HandlePayloadResult) { + unsafe { std::ptr::drop_in_place(result) } } impl From for HandlePayloadResult { @@ -310,7 +322,11 @@ pub struct NewConvoResult { } /// Free the result from create_new_private_convo +/// +/// # ABI note +/// Takes `&mut` instead of ownership because Nim always passes large structs as a pointer; +/// accepting the struct by value would be an ABI mismatch on the caller side. #[ffi_export] -pub fn destroy_convo_result(result: NewConvoResult) { - drop(result); +pub fn destroy_convo_result(result: &mut NewConvoResult) { + unsafe { std::ptr::drop_in_place(result) } } diff --git a/nim-bindings/src/libchat.nim b/nim-bindings/src/libchat.nim index 9b2cb0e..4097119 100644 --- a/nim-bindings/src/libchat.nim +++ b/nim-bindings/src/libchat.nim @@ -44,11 +44,10 @@ proc createIntroductionBundle*(ctx: LibChat): Result[seq[byte], string] = return err("Context handle is nil") let res = create_intro_bundle(ctx.handle) + defer: destroy_intro_result(res) if res.error_code != ErrNone: - result = err("Failed to create private convo: " & $res.error_code) - destroy_intro_result(res) - return + return err("Failed to create intro bundle: " & $res.error_code) return ok(res.intro_bytes.toSeq()) @@ -62,32 +61,18 @@ proc createNewPrivateConvo*(ctx: LibChat, bundle: seq[byte], content: seq[byte]) if content.len == 0: return err("content is zero length") - let res = bindings.create_new_private_convo( - ctx.handle, - bundle.toSlice(), - content.toSlice() - ) + let res = bindings.create_new_private_convo(ctx.handle, bundle.toSlice(), content.toSlice()) + defer: destroy_convo_result(res) if res.error_code != 0: - result = err("Failed to create private convo: " & $res.error_code) - destroy_convo_result(res) - return + return err("Failed to create private convo: " & $res.error_code) - # Convert payloads to Nim types var payloads = newSeq[PayloadResult](res.payloads.len) for i in 0 ..< res.payloads.len: let p = res.payloads[int(i)] - payloads[int(i)] = PayloadResult( - address: $p.address, - data: p.data.toSeq() - ) + payloads[int(i)] = PayloadResult(address: $p.address, data: p.data.toSeq()) - let convoId = $res.convo_id - - # Free the result - destroy_convo_result(res) - - return ok((convoId, payloads)) + return ok(($res.convo_id, payloads)) ## Send content to an existing conversation proc sendContent*(ctx: LibChat, convoId: string, content: seq[byte]): Result[seq[PayloadResult], string] = @@ -97,24 +82,13 @@ proc sendContent*(ctx: LibChat, convoId: string, content: seq[byte]): Result[seq if content.len == 0: return err("content is zero length") - let res = bindings.send_content( - ctx.handle, - convoId.toReprCString, - content.toSlice() - ) + let res = bindings.send_content(ctx.handle, convoId.toReprCString, content.toSlice()) + defer: destroy_send_content_result(res) if res.error_code != 0: - result = err("Failed to send content: " & $res.error_code) - destroy_send_content_result(res) - return + return err("Failed to send content: " & $res.error_code) - - let payloads = res.payloads.toSeq().mapIt(PayloadResult( - address: $it.address, - data: it.data.toSeq() - )) - - destroy_send_content_result(res) + let payloads = res.payloads.toSeq().mapIt(PayloadResult(address: $it.address, data: it.data.toSeq())) return ok(payloads) type @@ -131,14 +105,8 @@ proc handlePayload*(ctx: LibChat, payload: seq[byte]): Result[Option[ContentResu if payload.len == 0: return err("payload is zero length") - var conversationIdBuf = newSeq[byte](ctx.buffer_size) - var contentBuf = newSeq[byte](ctx.buffer_size) - var conversationIdLen: uint32 = 0 - - let res = bindings.handle_payload( - ctx.handle, - payload.toSlice(), - ) + let res = bindings.handle_payload(ctx.handle, payload.toSlice()) + defer: destroy_handle_payload_result(res) if res.error_code != ErrNone: return err("Failed to handle payload: " & $res.error_code)