-
Notifications
You must be signed in to change notification settings - Fork 33
fix!: pass the actual size of the buffer from the {Go,Rust} bindings #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis change adds an explicit Changes
Sequence Diagram(s)sequenceDiagram
actor Caller as Go/Python/Rust
participant C_WRAPPER as C bindings
participant BLS_LIB as bls::FromBytes
Note over Caller,C_WRAPPER: New argument added: len (size_t)
Caller->>C_WRAPPER: FromBytes(data_ptr, len, ... , &didErr)
alt success
C_WRAPPER->>BLS_LIB: Bytes(data_ptr, len) -> FromBytes(...)
BLS_LIB-->>C_WRAPPER: element_ptr
C_WRAPPER-->>Caller: element_ptr (didErr=false)
else failure
BLS_LIB--x C_WRAPPER: throws/error
C_WRAPPER-->>Caller: nullptr (didErr=true) and gErrMsg set
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code graph analysis (10)go-bindings/privatekey.cpp (2)
rust-bindings/bls-dash-sys/c-bindings/privatekey.cpp (1)
rust-bindings/bls-dash-sys/c-bindings/bip32/extendedprivatekey.cpp (3)
go-bindings/elements.cpp (2)
python-bindings/pythonbindings.cpp (2)
go-bindings/privatekey.go (1)
go-bindings/privatekey.h (1)
rust-bindings/bls-dash-sys/c-bindings/elements.h (3)
rust-bindings/bls-signatures/src/bip32/private_key.rs (2)
rust-bindings/bls-dash-sys/c-bindings/privatekey.h (3)
🔇 Additional comments (20)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
go-bindings/elements.cpp(2 hunks)go-bindings/elements.go(2 hunks)go-bindings/elements.h(2 hunks)go-bindings/privatekey.cpp(1 hunks)go-bindings/privatekey.go(1 hunks)go-bindings/privatekey.h(1 hunks)python-bindings/pythonbindings.cpp(10 hunks)rust-bindings/bls-dash-sys/bindings.rs(5 hunks)rust-bindings/bls-dash-sys/c-bindings/bip32/extendedprivatekey.cpp(2 hunks)rust-bindings/bls-dash-sys/c-bindings/bip32/extendedprivatekey.h(1 hunks)rust-bindings/bls-dash-sys/c-bindings/bip32/extendedpublickey.cpp(1 hunks)rust-bindings/bls-dash-sys/c-bindings/bip32/extendedpublickey.h(1 hunks)rust-bindings/bls-dash-sys/c-bindings/elements.cpp(2 hunks)rust-bindings/bls-dash-sys/c-bindings/elements.h(2 hunks)rust-bindings/bls-dash-sys/c-bindings/privatekey.cpp(1 hunks)rust-bindings/bls-dash-sys/c-bindings/privatekey.h(1 hunks)rust-bindings/bls-signatures/src/elements.rs(2 hunks)rust-bindings/bls-signatures/src/private_key.rs(1 hunks)
🔇 Additional comments (34)
go-bindings/privatekey.h (1)
26-26: LGTM! Length parameter added to enable runtime size validation.The signature now accepts the buffer length explicitly, allowing the implementation to validate against the actual buffer size instead of assuming a hardcoded constant. This addresses the issue mentioned in the PR where the binding previously always passed a buffer sized to the hardcoded constant, making in-library size checks ineffective.
rust-bindings/bls-dash-sys/c-bindings/bip32/extendedprivatekey.h (1)
19-22: LGTM! BIP32 extended key deserialization now length-aware.Consistent with the broader API evolution across this PR, the extended private key constructor now accepts an explicit length parameter.
rust-bindings/bls-dash-sys/c-bindings/privatekey.h (1)
26-26: LGTM! Signature updated consistently with Go bindings.The Rust C bindings now mirror the Go bindings with the explicit length parameter, ensuring consistent API across language bindings.
rust-bindings/bls-dash-sys/c-bindings/elements.h (2)
30-30: LGTM! G1 element deserialization now length-aware.
44-44: LGTM! G2 element deserialization now length-aware.Both G1 and G2 element constructors are updated consistently, enabling dynamic size validation for elliptic curve elements.
go-bindings/elements.h (2)
30-30: LGTM! Go G1 element constructor now accepts explicit length.
43-43: LGTM! Go G2 element constructor now accepts explicit length.Both Go element constructors updated consistently with the length-aware pattern.
python-bindings/pythonbindings.cpp (12)
49-49: LGTM! Using dynamic size for buffer copy.The change from a hardcoded constant to
data.size()improves consistency and reduces the risk of size mismatches. Since the array is already correctly sized at line 48, this is safe.
363-363: LGTM! Using actual buffer size for PyLong_AsByteArray.Replacing the hardcoded
G1Element::SIZEwithbuffer.size()ensures consistency and safety.
383-383: LGTM! Dynamic sizing for G1Element buffer copy.
401-401: LGTM! Dynamic sizing for G1Element from_bytes.
512-512: LGTM! Dynamic sizing for G2Element buffer copy.
521-521: LGTM! Using actual buffer size for G2Element PyLong_AsByteArray.
543-543: LGTM! Dynamic sizing for G2Element from_bytes.
640-640: Bug fix: Corrected error message for GTElement.Good catch! The error message previously incorrectly referenced
G2Element::SIZEinstead ofGTElement::SIZE.
644-644: LGTM! Dynamic sizing for GTElement buffer copy.
649-653: LGTM! Consistent GTElement buffer initialization and sizing.Lines 649 and 653 now explicitly use
GTElement::SIZEandbuffer.size()respectively, improving consistency across GTElement constructors.
675-675: LGTM! Dynamic sizing for GTElement from_bytes.
693-693: LGTM! Dynamic sizing for GTElement from_bytes_unchecked.All Python binding changes consistently replace hardcoded sizes with dynamic container sizes, improving safety and maintainability.
rust-bindings/bls-signatures/src/elements.rs (2)
78-78: LGTM! FFI call updated to pass actual byte slice length.The call now passes
bytes.len()as the second argument, aligning with the updated C API. The length validation at lines 67-75 ensures only correctly-sized buffers reach the FFI boundary.
237-237: LGTM! G2Element FFI call updated consistently.Both G1 and G2 element deserialization now pass the actual buffer length to the C layer, with pre-validation ensuring correctness.
rust-bindings/bls-dash-sys/c-bindings/bip32/extendedpublickey.h (1)
17-21: LGTM! BIP32 extended public key now length-aware.Consistent with the corresponding extended private key update, ensuring both BIP32 key types benefit from explicit length validation.
rust-bindings/bls-dash-sys/c-bindings/bip32/extendedpublickey.cpp (1)
11-19: Explicit length propagation keeps FromBytes safe. Passinglenthrough tobls::Bytesensures the library’s internal bounds check operates on the actual buffer size. Looks good.go-bindings/elements.go (2)
43-44: Go G1 bindings now forward the true slice length. UsingC.size_t(len(data))keeps the C layer in sync with the Go slice size, so the validation isn’t bypassed.
134-135: Same solid fix for G2. The length-aware call path mirrors the G1 change and maintains symmetry across element bindings.rust-bindings/bls-signatures/src/private_key.rs (1)
140-141: Rust FFI call stays consistent with the new C signature. Forwardingbytes.len()lines up the safe Rust guard with the runtime length check underneath.go-bindings/privatekey.cpp (1)
23-39: Private key FromBytes now honours caller-provided buffer size. The newlenparameter keeps the underlying bounds validation meaningful.rust-bindings/bls-dash-sys/c-bindings/privatekey.cpp (1)
23-39: Dash-sys bridge mirrors the length-aware constructor. Propagatinglenhere keeps the Rust bindings and lower layer aligned.go-bindings/privatekey.go (1)
41-42: Go wrapper lines up with the updated C signature. Feedinglen(data)through CGO ensures the buffer length check holds end to end.rust-bindings/bls-dash-sys/c-bindings/elements.cpp (2)
26-31: G1 element path now respects the caller-provided length. Matching the new signature keeps the validation active across languages.
100-105: G2 element path follows suit. Propagatinglenhere maintains consistent safety coverage for signatures as well.go-bindings/elements.cpp (2)
26-31: C shim for G1 forwards the correct length. This pairs cleanly with the Go-side changes and restores the size guard.
96-101: G2 shim mirrors the fix. Length propagation here keeps the signature deserialization path robust.rust-bindings/bls-dash-sys/c-bindings/bip32/extendedprivatekey.cpp (1)
24-29: LGTM! Consistent with FromBytes changes.The
FromSeedfunction already had alenparameter, and this change ensures it's properly used in thebls::Bytesconstructor, maintaining consistency with theFromBytesfunction.
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK f7e3f00
The bindings are currently taking the buffer as-is and assuming it is of correct length. This makes the internal size check entirely useless. Let the creator of the buffer pass the length instead of assuming the buffer is well formed. BREAKING CHANGE: Modifies function signature in Rust bindings
BREAKING CHANGE: Modifies function signature in Go bindings
The base branch was changed.
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM a47caad
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK a47caad
…169ee9c as 83d0727 0ff505c build: stop tracking cmake dependency relic_conf.h.in (Kittywhiskers Van Gogh) 83d0727 Squashed 'src/dashbls/' changes from dd683653c6..6169ee9c91 (Kittywhiskers Van Gogh) 61a1f72 revert: stop tracking cmake dependency relic_conf.h.in (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Expected subtree hash `fd3a6b99a590fadd699fe9bad2336679d351ca5604938996da4e211f77d962af` (see [instructions](#6323 (review)) to calculate) * Includes the following changes * dashpay/bls-signatures#117 * dashpay/bls-signatures#118 * dashpay/bls-signatures#116 * dashpay/bls-signatures#119 ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 0ff505c PastaPastaPasta: utACK 0ff505c Tree-SHA512: 8430e6ee36990344cac49a112dfd0cc76083b0b4991a9213872f54982f4c106e544b065bedee47ab9f928f8650787de83d55c232a32050c07b2100f428963424
Additional Information
The enablement of macOS bindings revealed a potential cause for concern (build). The binds so far take in a buffer and assume it has a sane size.
bls-signatures/go-bindings/elements.cpp
Lines 26 to 31 in dd68365
Which makes the in-library check useless as the condition will always be met.
bls-signatures/src/elements.cpp
Lines 34 to 36 in dd68365
To resolve this, when specifying a buffer, you must also specify the size, preferably reported by the object controlling the buffer (like
buf.size()) instead of hardcoding it (likePrivateKey::PRIVATE_KEY_SIZE).In bls-signatures#117, CodeRabbit identified a typo in the Python bindings (source) utilising the wrong
SIZEvariable. This has been resolved.Breaking changes
The introduction of a
sizeparameter may require updating function calls to affected bindings.Summary by CodeRabbit
Release Notes