ffi: wrap BN_hex2bn in C to avoid BIGNUM** ctypes binding#2
Open
ffi: wrap BN_hex2bn in C to avoid BIGNUM** ctypes binding#2
Conversation
Replace direct ctypes binding of BN_hex2bn (BIGNUM**) with a small C wrapper returning BIGNUM*. This avoids void**/BIGNUM** incompatibility (GCC15+) and simplifies the OCaml call site. No change in behavior: still returns None on failure and allocates a new BIGNUM on success.
Author
|
To validate the change compiles under GCC 15:
Before this patch, GCC 15 compilation was failing. |
Member
|
I'm also concerned that we might need to maintain multiple version of this package if there's some future bugfixes we need to apply while supporting both. |
Member
Member
|
I'd consider #3 instead if acceptable |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This patch replaces the direct
ctypesbinding ofBN_hex2bnwith a small C wrapper that returns aBIGNUM *instead of requiring aBIGNUM **out-parameter.This resolves a type incompatibility introduced by stricter compilers (e.g. GCC 15) and simplifies the OCaml call site, while preserving the exact runtime behavior.
The issue
The OpenSSL API for parsing hex into a bignum is:
This function uses an out-parameter (
BIGNUM **) with the following semantics:*a == NULL→ allocate a newBIGNUM*a != NULL→ reuse existingBIGNUMreturn value:
0→ failure>0→ number of hex digits parsedOriginal binding
Previously, this was bound in
ctypesas:and used as:
This corresponds to the C pattern:
which is the correct usage of the OpenSSL API.
Problem with newer compilers
Internally,
ctypesgenerated a stub passing avoid **toBN_hex2bn, because the OCaml representation of the pointer was erased tovoid *.This results in code like:
However,
void **is not compatible withBIGNUM **in C. While historically tolerated, this is rejected by newer compilers (e.g. GCC 15):This is a fundamental limitation of representing typed double pointers via
ctypes.Approach
Instead of binding
BN_hex2bndirectly, this patch introduces a small C wrapper:This transforms the API from:
into:
The OCaml binding is then updated to:
and the call site simplified to:
Why this is correct and safe
1. Correct use of OpenSSL API
The wrapper uses the standard and documented allocation pattern:
Passing a pointer initialized to
NULLexplicitly signals:This is one of the two intended modes of
BN_hex2bn.2. Preserves behavior exactly
The old and new implementations are equivalent:
*a == NULLviaallocate Nonebn = NULLin wrapperNoneafter dereferencing pointerNULL→NoneSome pSome pThe only internal difference is that:
== 0and returnsNULLThis does not change observable behavior.
3. Fixes the
void **vsBIGNUM **issueBy removing
BIGNUM **from the FFI boundary entirely, the generated stubs now only deal with:which is safely representable via
ctypesas an opaque pointer.This eliminates the incompatible pointer type error on newer compilers.
4. Keeps
BIGNUMfully opaqueThe OCaml side still treats
BIGNUM *as an opaque handle:BN_free)This matches OpenSSL’s design and avoids any reliance on internal representation.
5. Memory ownership is unchanged
In both versions:
BN_hex2bnallocates a newBIGNUMBN_freeThe wrapper does not introduce any additional allocations or leaks.
6. Simpler and safer OCaml call site
The OCaml code no longer needs to:
ptr t_opt)This reduces surface area for mistakes and makes the intent clearer.
Conclusion
This patch:
void **vsBIGNUM **)It is a minimal and safe fix for the issue.