From 83f1aae895a7fb7d1db4f75e86103ffbade11992 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com> Date: Tue, 28 Jul 2026 10:04:13 -0300 Subject: [PATCH] fix: non-canonical seq CBOR generation (#141) --- ffi/codegen/rust.nim | 39 +++++++++++++++++--- ffi/internal/ffi_macro.nim | 7 ++-- tests/unit/test_ffi_context.nim | 58 ++++++++++++++++++++++++++++++ tests/unit/test_rust_codegen.nim | 62 ++++++++++++++++++++++++++++++-- tests/unit/test_wire_compat.nim | 47 ++++++++++++++++++++++++ 5 files changed, 202 insertions(+), 11 deletions(-) diff --git a/ffi/codegen/rust.nim b/ffi/codegen/rust.nim index eba0d13..7e41a8f 100644 --- a/ffi/codegen/rust.nim +++ b/ffi/codegen/rust.nim @@ -30,7 +30,9 @@ func rustOpt(elem: string): string = const rustMap = NativeTypeMap( scalar: rustScalar, str: "String", - bytes: "Vec", + # serde encodes a plain Vec as a CBOR integer array, which Nim rejects. + # ByteBuf gives the CBOR byte string that Nim decodes. + bytes: "serde_bytes::ByteBuf", ptrType: RustPtrType, seqOf: rustSeq, optOf: rustOpt, @@ -68,8 +70,33 @@ proc reqStructName(p: FFIProcMeta): string = else: camel & "Req" -proc generateCargoToml*(libName: string): string = +func typeUsesBytes(typeName: string): bool = + ## True if `typeName` resolves to a `seq[byte]` at any depth of Seq or Option. + var t = parseFFIType(typeName) + while t.kind in {ftSeq, ftOpt}: + t = t.elem + t.kind == ftBytes + +func needsSerdeBytes*(types: seq[FFITypeMeta], procs: seq[FFIProcMeta]): bool = + ## True if a field, a parameter or a return type maps to `serde_bytes::ByteBuf`. + ## `types` holds every struct, thus a scan of the fields also finds the bytes + ## in a nested struct. + for t in types: + for f in t.fields: + if typeUsesBytes(f.typeName): + return true + for p in procs: + for ep in p.extraParams: + if typeUsesBytes(ep.typeName): + return true + if p.returnTypeName.len > 0 and typeUsesBytes(p.returnTypeName): + return true + false + +proc generateCargoToml*(libName: string, needsBytes = false): string = # flume: callback channel (recv_timeout + recv_async), default-features off. tokio: only the async timeout. + # Add serde_bytes only when a `seq[byte]` goes on the wire as a CBOR byte string. + let serdeBytesDep = if needsBytes: "\nserde_bytes = \"0.11\"" else: "" return """[package] name = "$1" @@ -77,7 +104,7 @@ version = "0.1.0" edition = "2021" [dependencies] -serde = { version = "1", features = ["derive"] } +serde = { version = "1", features = ["derive"] }$2 ciborium = "0.2" flume = { version = "0.11", default-features = false, features = ["async"] } tokio = { version = "1", features = ["sync", "time"] } @@ -85,7 +112,7 @@ tokio = { version = "1", features = ["sync", "time"] } [dev-dependencies] tokio = { version = "1", features = ["rt-multi-thread", "macros", "sync", "time"] } """ % - [libName] + [libName, serdeBytesDep] proc generateBuildRs*(libName: string, nimSrcRelPath: string): string = ## Generates build.rs that compiles the Nim library; nimSrcRelPath is relative @@ -788,7 +815,9 @@ proc generateRustCrate*( createDir(outputDir) createDir(outputDir / "src") - writeFile(outputDir / "Cargo.toml", generateCargoToml(libName)) + writeFile( + outputDir / "Cargo.toml", generateCargoToml(libName, needsSerdeBytes(types, procs)) + ) writeFile(outputDir / "build.rs", generateBuildRs(libName, nimSrcRelPath)) writeFile(outputDir / "src" / "lib.rs", generateLibRs()) writeFile(outputDir / "src" / "ffi.rs", generateFFIRs(procs)) diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 2915811..71590f9 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -524,9 +524,10 @@ proc replyEncode( return ok(cwireStructBytes(`wireIdent`)) return quote: - when typeof(`typedResIdent`.value) is seq[byte]: - return ok(`typedResIdent`.value) - elif typeof(`typedResIdent`.value) is void: + # A `seq[byte]` result goes on the wire as CBOR, the same as every other + # `abi = cbor` return. The C, C++ and Rust decoders expect CBOR. They reject + # raw bytes with "value encoded in non-canonical form". + when typeof(`typedResIdent`.value) is void: return ok(newSeq[byte]()) elif typeof(`typedResIdent`.value) is FFIHandleRoot: return ok( diff --git a/tests/unit/test_ffi_context.nim b/tests/unit/test_ffi_context.nim index 41bdedf..15f38c4 100644 --- a/tests/unit/test_ffi_context.nim +++ b/tests/unit/test_ffi_context.nim @@ -75,6 +75,14 @@ registerReqFFI(SlowRequest, lib: ptr TestLib): await sleepAsync(500.milliseconds) return ok("slow-done") +registerReqFFI(BytesReplyRequest, lib: ptr TestLib): + proc(): Future[Result[seq[byte], string]] {.async.} = + return ok(@[0xDE'u8, 0xAD'u8, 0xBE'u8, 0xEF'u8]) + +registerReqFFI(EmptyBytesReplyRequest, lib: ptr TestLib): + proc(): Future[Result[seq[byte], string]] {.async.} = + return ok(newSeq[byte]()) + var gSyncBlockStarted: Channel[bool] gSyncBlockStarted.open() @@ -320,6 +328,56 @@ suite "sendRequestToFFIThread": check d.retCode == RET_ERR check callbackErr(d) == "intentional failure" + test "seq[byte] result rides as a CBOR byte string, not raw bytes": + # A `seq[byte]` return must be CBOR, the same as every other reply. The + # generated C, C++ and Rust decoders call `nimffi_dec_bytes` on the payload + # and reject a raw reply with "value encoded in non-canonical form". + var d: CallbackData + initCallbackData(d) + defer: + deinitCallbackData(d) + + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: + check false + return + defer: + discard pool.destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, BytesReplyRequest.ffiNewReq(testCallback, addr d)) + .isOk() + waitCallback(d) + check d.retCode == RET_OK + let reply = callbackBytes(d) + # The wire contract is a CBOR byte-string header (major type 2, 0x40..0x5b) + # and then the 4 payload bytes. + check reply.len == 5 + check reply[0] == 0x44'u8 + check cborDecode(reply, seq[byte]).value == @[0xDE'u8, 0xAD'u8, 0xBE'u8, 0xEF'u8] + + test "empty seq[byte] result rides as an empty CBOR byte string": + var d: CallbackData + initCallbackData(d) + defer: + deinitCallbackData(d) + + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: + check false + return + defer: + discard pool.destroyFFIContext(ctx) + + check sendRequestToFFIThread( + ctx, EmptyBytesReplyRequest.ffiNewReq(testCallback, addr d) + ) + .isOk() + waitCallback(d) + check d.retCode == RET_OK + let reply = callbackBytes(d) + check reply == @[0x40'u8] # byte string, length 0 + check cborDecode(reply, seq[byte]).value.len == 0 + test "empty ok response delivers empty message": var d: CallbackData initCallbackData(d) diff --git a/tests/unit/test_rust_codegen.nim b/tests/unit/test_rust_codegen.nim index e01c8e8..5db54b6 100644 --- a/tests/unit/test_rust_codegen.nim +++ b/tests/unit/test_rust_codegen.nim @@ -1,8 +1,9 @@ ## Regression tests for the Rust type mapping: `nimTypeToRust` renders through ## the shared `parseFFIType` IR, so the full scalar set is pinned here. +import std/strutils import unittest2 -import ffi/codegen/rust +import ffi/codegen/[rust, meta] suite "nimTypeToRust: scalar set": test "every scalar maps to its Rust primitive (the drift that regressed)": @@ -27,14 +28,69 @@ suite "nimTypeToRust: strings, pointers and containers": check nimTypeToRust("string") == "String" check nimTypeToRust("cstring") == "String" - test "seq[byte] collapses to Vec and ptr/pointer to the wire int": - check nimTypeToRust("seq[byte]") == "Vec" + test "seq[byte] rides as serde_bytes::ByteBuf (CBOR byte string) and ptr/pointer to the wire int": + check nimTypeToRust("seq[byte]") == "serde_bytes::ByteBuf" + check nimTypeToRust("seq[uint8]") == "serde_bytes::ByteBuf" check nimTypeToRust("ptr Foo") == RustPtrType check nimTypeToRust("pointer") == RustPtrType test "generics nest and Maybe aliases Option": check nimTypeToRust("seq[Option[int8]]") == "Vec>" check nimTypeToRust("Maybe[uint16]") == "Option" + check nimTypeToRust("seq[seq[byte]]") == "Vec" + check nimTypeToRust("Option[seq[byte]]") == "Option" test "an unknown user type is capitalised, not mistaken for a scalar": check nimTypeToRust("echoRequest") == "EchoRequest" + +suite "generateTypesRs: seq[byte] rides as a CBOR byte string": + ## A `seq[byte]` in a struct that is a `seq` element became a `Vec` integer + ## array (CBOR major type 4). The Nim decoder rejects that array with "value + ## encoded in non-canonical form". ByteBuf makes ciborium write a byte string + ## (major type 2), the same as every other backend. + setup: + let types = @[ + FFITypeMeta( + name: "ServiceInfoEntry", + fields: @[ + FFIFieldMeta(name: "id", typeName: "string"), + FFIFieldMeta(name: "data", typeName: "seq[byte]"), + ], + ), + FFITypeMeta( + name: "CreateXprRequest", + fields: @[FFIFieldMeta(name: "services", typeName: "seq[ServiceInfoEntry]")], + ), + ] + let procs = @[ + FFIProcMeta( + procName: "lib_create_xpr", + libName: "lib", + kind: FFIKind.FFI, + libTypeName: "Lib", + extraParams: @[FFIParamMeta(name: "req", typeName: "CreateXprRequest")], + returnTypeName: "seq[byte]", + ) + ] + let rs = generateTypesRs(types, procs) + + test "a nested seq[byte] struct field becomes serde_bytes::ByteBuf": + check "pub data: serde_bytes::ByteBuf," in rs + check "pub data: Vec," notin rs + + test "the seq-of-struct wrapper stays a Vec of the struct": + check "pub services: Vec," in rs + + test "Cargo.toml pulls in serde_bytes only when a type needs a byte string": + check needsSerdeBytes(types, procs) + check "serde_bytes = " in generateCargoToml("lib", needsBytes = true) + check "serde_bytes = " notin generateCargoToml("lib", needsBytes = false) + + test "a byte-less registry keeps serde_bytes out of Cargo.toml": + let plain = @[ + FFITypeMeta( + name: "Plain", fields: @[FFIFieldMeta(name: "name", typeName: "string")] + ) + ] + check not needsSerdeBytes(plain, @[]) + check "serde_bytes" notin generateCargoToml("lib", needsSerdeBytes(plain, @[])) diff --git a/tests/unit/test_wire_compat.nim b/tests/unit/test_wire_compat.nim index 9d58cc6..d378baa 100644 --- a/tests/unit/test_wire_compat.nim +++ b/tests/unit/test_wire_compat.nim @@ -22,6 +22,16 @@ type WireWithVector {.ffi.} = object type WireWithBytes {.ffi.} = object blob: seq[byte] +type WireBytesEntry {.ffi.} = object + ## A struct with a `seq[byte]` field. The type below uses it as a seq element. + id: string + data: seq[byte] + +type WireNestedBytes {.ffi.} = object + ## A `seq[byte]` in a struct that is a `seq` element. This shape broke the + ## Rust backend, which wrote an integer array in place of a ByteBuf. + entries: seq[WireBytesEntry] + proc toHex(bytes: openArray[byte]): string = var buf = "" for b in bytes: @@ -92,3 +102,40 @@ suite "wire format — seq[byte]": let v = WireWithBytes(blob: @[]) let bytes = cborEncode(v) check toHex(bytes) == "a164626c6f6240" + +suite "wire format — seq[byte] nested in a seq-of-struct": + ## A `seq[byte]` field in a struct that is a `seq` element must stay a CBOR + ## byte string (major type 2) at depth. Every backend must match this wire + ## contract. The Rust generator broke it and wrote a `Vec` integer array. + test "each nested seq[byte] rides as a byte string, request and response alike": + let v = WireNestedBytes( + entries: @[ + WireBytesEntry(id: "s0", data: @[0xAA'u8, 0xBB'u8]), + WireBytesEntry(id: "s1", data: @[1'u8, 2'u8, 3'u8]), + ] + ) + let bytes = cborEncode(v) + check toHex(bytes) == + "a167656e747269657382a2626964627330646461746142aabba26269646273316464617461" & + "43010203" + # The payloads must use byte-string headers (0x42 = bytes(2), 0x43 = + # bytes(3)), not array headers (0x82 or 0x83). + check "6461746142aabb" in toHex(bytes) # "data" + 0x42 + check "6461746143010203" in toHex(bytes) # "data" + 0x43 <01 02 03> + let back = cborDecode(bytes, WireNestedBytes) + check back.isOk + check back.value.entries.len == 2 + check back.value.entries[0].data == @[0xAA'u8, 0xBB'u8] + check back.value.entries[1].data == @[1'u8, 2'u8, 3'u8] + + test "empty and multi-byte-length nested seq[byte] round-trip": + let v = WireNestedBytes( + entries: @[ + WireBytesEntry(id: "", data: @[]), + WireBytesEntry(id: "big", data: newSeq[byte](30)), + ] + ) + let back = cborDecode(cborEncode(v), WireNestedBytes) + check back.isOk + check back.value.entries[0].data.len == 0 + check back.value.entries[1].data.len == 30