fix(rs-sdk-ffi): shrink signature allocation to len before leaking (capacity UB)#3798
Conversation
…apacity UB) `dash_sdk_signer_sign` did `signature.to_vec().leak()`, which leaks the buffer keeping the source Vec's actual capacity, while `dash_sdk_signature_free` reconstructs it with `Vec::from_raw_parts(ptr, len, len)`. `from_raw_parts` requires the exact original capacity; when the source `BinaryData`'s inner Vec had `capacity > len`, the global allocator received a wrong `Layout` on dealloc — undefined behavior / heap corruption. This is the same bug class already fixed in `DashSDKResult::success_binary` via `into_boxed_slice()`. The signing path now routes through a small `leak_binary_to_ptr` helper that shrinks capacity to len (`into_boxed_slice()`) before leaking, making the `cap == len` free path sound. Audited the rest of rs-sdk-ffi: this was the only `.leak()`; all other `from_raw_parts(ptr, len, len)` sites already consume exact-capacity allocations. Public `extern "C"` ABI and `DashSDKSignature` are unchanged. Adds a regression test driving a `capacity > len` buffer through the real alloc + free round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete (commit 7edf6ba) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR fixes an FFI memory safety issue in Rust-SDK signature serialization. A new ChangesFFI Memory Safety for Signature Serialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Tight, ABI-preserving soundness fix for a real latent UB in dash_sdk_signer_sign. The previous signature.to_vec().leak() retained the source Vec's capacity while dash_sdk_signature_free reclaimed via Vec::from_raw_parts(ptr, len, len) — a layout mismatch whenever cap > len. The new leak_binary_to_ptr shrinks via into_boxed_slice() so the free path is sound; the pattern mirrors the already-shipped DashSDKResult::success_binary fix, and regression/empty/null-free tests all pass verification. All six agents agree no in-scope issues remain.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "97ab878a4437e55f9ba7ada95999988bf04ffd3312a91ce8a0889cb63e794d25"
)Xcode manual integration:
|
Issue being fixed or feature implemented
Latent heap-corruption UB on the native signer's sign → free path.
dash_sdk_signer_sign(signer_simple.rs) returned the signature via:while
dash_sdk_signature_freereclaims it withVec::from_raw_parts(sig.signature, sig.signature_len, sig.signature_len).Vec::from_raw_parts(ptr, len, cap)requirescapto exactly equal the original allocation capacity.signature.to_vec()clones aBinaryData's innerVec, whose capacity is not guaranteed to equal its length — whencapacity > len, the global allocator is handed a wrongLayouton dealloc → undefined behavior / heap corruption.This is the same bug class already fixed in
DashSDKResult::success_binary(types.rs) viainto_boxed_slice(), with a regression test noting it "was previously UB when capacity != len". The signer path never got the fix.What was done?
The signing arm now routes through a small private helper that shrinks capacity to len before leaking:
A
Box<[u8]>of lengthlenowns an allocation of exactlylenbytes with aVec-compatible layout, so the existingVec::from_raw_parts(ptr, len, len)free is now sound. The free fn and the#[repr(C)] DashSDKSignaturestruct are unchanged.I also audited the rest of
rs-sdk-ffi: this was the only.leak()in the crate, and every otherVec::from_raw_parts(ptr, len, len)site already consumes an exact-capacity allocation, so no other instances of this bug exist.How Has This Been Tested?
cargo test -p rs-sdk-ffi --lib signer_simple— 6 passed, 0 failed, including the newtest_leak_binary_to_ptr_preserves_capacity_via_shrink, which builds a 65-byte signature inside aVec::with_capacity(128)(capacity 128 > len 65 — the exact mismatch the old.leak()mishandled), runs it throughleak_binary_to_ptr+ the realdash_sdk_signature_free, and asserts the bytes round-trip without corrupting the heap. The existingtest_success_binary_preserves_capacity_via_shrinkstill passes.cargo fmt --allclean.Breaking Changes
None. Internal allocation-layout correctness fix;
extern "C"ABI unchanged.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Tests