fix: non-canonical seq CBOR generation (#141)

ffi/codegen/rust.nim

@@ -30,7 +30,9 @@ func rustOpt(elem: string): string =
 const rustMap = NativeTypeMap(
   scalar: rustScalar,
   str: "String",
-  bytes: "Vec<u8>",
+  # serde encodes a plain Vec<u8> 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))

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(

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)

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<u8> and ptr/pointer to the wire int":
-    check nimTypeToRust("seq[byte]") == "Vec<u8>"
+  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<Option<i8>>"
     check nimTypeToRust("Maybe[uint16]") == "Option<u16>"
+    check nimTypeToRust("seq[seq[byte]]") == "Vec<serde_bytes::ByteBuf>"
+    check nimTypeToRust("Option[seq[byte]]") == "Option<serde_bytes::ByteBuf>"
 
   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<u8>` 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<u8>," notin rs
+
+  test "the seq-of-struct wrapper stays a Vec of the struct":
+    check "pub services: Vec<ServiceInfoEntry>," 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, @[]))

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<u8>` 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 <AA BB>
+    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