Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label merge-ready to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
08e7cb0 to
d9e5fe8
Compare
| # Core dependencies for serialization | ||
| binius-core = { path = "../core" } | ||
| binius-utils = { path = "../utils" } | ||
| bytes = "1.7" |
There was a problem hiding this comment.
The workspace configuration requires all crates to reference dependencies declared in the workspace root Cargo.toml. The dependency bytes = "1.7" should be declared in the workspace root and referenced here as bytes.workspace = true.
Spotted by Diamond (based on custom rule: Irreducible Rust and Cargo)
Is this helpful? React 👍 or 👎 to let us know.
| priv_witness_bytes.len(), | ||
| log_inv_rate, | ||
| proof_buf.as_mut_ptr(), | ||
| proof_buf.capacity(), |
There was a problem hiding this comment.
Security Issue: Using proof_buf.capacity() in the FFI call creates a potential buffer overflow vulnerability. While both capacity and length are currently identical (since the vector is initialized with a fixed size), this code would be unsafe if the vector implementation changes.
Replace with proof_buf.len() to correctly represent the actual usable buffer size:
let result = binius_prove(
// other parameters...
proof_buf.as_mut_ptr(),
proof_buf.len(), // Use len() instead of capacity()
);This ensures the FFI function never writes beyond the initialized portion of the buffer, preventing potential memory corruption.
| proof_buf.capacity(), | |
| proof_buf.len(), |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| let cs_slice = std::slice::from_raw_parts(cs_bytes, cs_len); | ||
| let pub_witness_slice = std::slice::from_raw_parts(pub_witness_bytes, pub_witness_len); | ||
| let priv_witness_slice = std::slice::from_raw_parts(priv_witness_bytes, priv_witness_len); |
There was a problem hiding this comment.
The slice creation from raw pointers lacks validation for memory safety. Since these slices are created in an unsafe block, there should be additional checks to ensure the provided lengths don't exceed allocated memory regions. Consider adding bounds checking or explicitly documenting in the function's safety documentation that callers must guarantee valid memory regions and accurate lengths. This is particularly important for FFI functions where invalid inputs from external callers could lead to undefined behavior or security vulnerabilities.
| let cs_slice = std::slice::from_raw_parts(cs_bytes, cs_len); | |
| let pub_witness_slice = std::slice::from_raw_parts(pub_witness_bytes, pub_witness_len); | |
| let priv_witness_slice = std::slice::from_raw_parts(priv_witness_bytes, priv_witness_len); | |
| // SAFETY: Caller must ensure that: | |
| // - All pointers are valid for reads of their respective lengths | |
| // - All pointers are properly aligned for u8 | |
| // - The memory regions won't be mutated for the lifetime of these slices | |
| // - The lengths accurately represent the allocated memory regions | |
| let cs_slice = std::slice::from_raw_parts(cs_bytes, cs_len); | |
| let pub_witness_slice = std::slice::from_raw_parts(pub_witness_bytes, pub_witness_len); | |
| let priv_witness_slice = std::slice::from_raw_parts(priv_witness_bytes, priv_witness_len); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.

Implements ENG2-282. Prototype for a FFI-based prover client.