fix: drain error queue in _get_key_parameters() on 3.x#164
Merged
timlegge merged 1 commit intoApr 5, 2026
Merged
Conversation
Member
|
@Koan-Bot this looks like it can close |
Member
|
@Koan-Bot rebase from main |
EVP_PKEY_get_bn_param() pushes errors onto the OpenSSL error queue when a parameter is absent (e.g. private components on a public key). These stale errors could leak into the next croakSsl() call from an unrelated operation. Add ERR_clear_error() after the parameter extraction calls. The API contract is preserved: missing parameters return undef (via cor_bn2sv(NULL)), matching the pre-3.x behavior exactly. Add t/get_key_parameters.t to codify this contract: private keys return 8 defined values, public keys return n/e defined + 6 undef. Addresses review feedback on PR cpan-authors#159. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Rebase: fix: drain error queue in _get_key_parameters() on 3.xBranch Diff: 2 files changed, 4 insertions(+), 1 deletion(-) Review feedback was analyzed and applied. Actions
CICI will be checked asynchronously. Automated by Kōan |
Koan-Bot
force-pushed
the
koan.atoomic/get-key-params-no-croak
branch
from
5cca67c to
f0655eb
Compare
timlegge
approved these changes
Apr 5, 2026
Member
timlegge
left a comment
timlegge
left a comment
There was a problem hiding this comment.
Approved comment change and MANIFEST update
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
2 participants
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.
What
Add
ERR_clear_error()after theEVP_PKEY_get_bn_param()calls in_get_key_parameters()on the OpenSSL 3.x path, and add a test codifying the undef-for-missing-params contract.Why
Per review feedback on PR #159 from @timlegge: the contracted API returns undef for any parameter that cannot be retrieved — it should not croak. The current code already does this correctly (NULL →
cor_bn2sv(NULL)→ undef), but failedEVP_PKEY_get_bn_param()calls push errors onto the OpenSSL error queue that could leak into the nextcroakSsl()call from an unrelated operation.How
ERR_clear_error()after all 8EVP_PKEY_get_bn_param()calls to drain stale errorst/get_key_parameters.t(10 tests) codifies the API contract for both private and public keysTesting
Supersedes #159. Addresses review feedback from @timlegge.
🤖 Generated with Claude Code
Quality Report
Changes: 3 files changed, 51 insertions(+)
Code scan: clean
Tests: passed (OK)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline