Constify the X509_STORE_CTX argument to the lookup_certs functions.#29488
Closed
bob-beck wants to merge 1 commit intoopenssl:masterfrom
Closed
Constify the X509_STORE_CTX argument to the lookup_certs functions.#29488bob-beck wants to merge 1 commit intoopenssl:masterfrom
bob-beck wants to merge 1 commit intoopenssl:masterfrom
Conversation
The justification for this not being const was because of lookup_certs_sk(). The reasons this function could not have a const store, is that it set the ctx's error code when we could not allocate memory and returned NULL. However, the other lookup_certs function, X509_STORE_CTX_get1_certs, already does not set this error code when failing to allocate memory on a return. Given that you can't depend on the out of memory error code being set in the general case, and the Beyonce rule appears to indicate that nobody likes this behaviour (as nobody put a test on it) I think it's safe to say we should just not modify the ctx, and constify it. For openssl#28654
bob-beck
changed the title
bob-beck
commented
Dec 22, 2025
| if (X509_NAME_cmp(nm, X509_get_subject_name(x)) == 0) { | ||
| if (!X509_add_cert(sk, x, X509_ADD_FLAG_UP_REF)) { | ||
| OSSL_STACK_OF_X509_free(sk); | ||
| ctx->error = X509_V_ERR_OUT_OF_MEM; |
Contributor
Author
There was a problem hiding this comment.
Note that X509_STORE_CTX_get1_certs returns NULL on failure to allocate without setting this error.
github-actions
bot
added
severity: fips change
2 tasks
nhorman
approved these changes
Dec 22, 2025
Contributor
|
Typo in commit message : funcitons |
jogme
approved these changes
Jan 13, 2026
Contributor
jogme
left a comment
jogme
left a comment
There was a problem hiding this comment.
please typofix the commit msg as above, otherwise LGTM! Thanks!
jogme
added
approval: done
openssl-machine
added
approval: ready to merge
Collaborator
|
This pull request is ready to merge |
paulidale
approved these changes
Jan 14, 2026
openssl-machine
pushed a commit
that referenced
this pull request
Jan 19, 2026
The justification for this not being const was because of lookup_certs_sk(). The reasons this function could not have a const store, is that it set the ctx's error code when we could not allocate memory and returned NULL. However, the other lookup_certs function, X509_STORE_CTX_get1_certs, already does not set this error code when failing to allocate memory on a return. Given that you can't depend on the out of memory error code being set in the general case, and the Beyonce rule appears to indicate that nobody likes this behaviour (as nobody put a test on it) I think it's safe to say we should just not modify the ctx, and constify it. For #28654 Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Norbert Pocs <[email protected]> Reviewed-by: Paul Dale <[email protected]> MergeDate: Mon Jan 19 12:03:05 2026 (Merged from #29488)
Member
|
Merged to master. Thank you for the contribution. |
esyr
pushed a commit
to esyr/openssl
that referenced
this pull request
Jan 19, 2026
The justification for this not being const was because of lookup_certs_sk(). The reasons this function could not have a const store, is that it set the ctx's error code when we could not allocate memory and returned NULL. However, the other lookup_certs function, X509_STORE_CTX_get1_certs, already does not set this error code when failing to allocate memory on a return. Given that you can't depend on the out of memory error code being set in the general case, and the Beyonce rule appears to indicate that nobody likes this behaviour (as nobody put a test on it) I think it's safe to say we should just not modify the ctx, and constify it. For openssl#28654 Reviewed-by: Neil Horman <[email protected]> Reviewed-by: Norbert Pocs <[email protected]> Reviewed-by: Paul Dale <[email protected]> MergeDate: Mon Jan 19 12:03:05 2026 (Merged from openssl#29488)
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
7 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.
The justification for this not being const was because of lookup_certs_sk(). The reasons this function could not have a const store, is that it set the ctx's error code
when we could not allocate memory and returned NULL.
However, the other lookup_certs function, X509_STORE_CTX_get1_certs, already does not set this error code when failing to allocate memory on a return.
Given that you can't depend on the out of memory error code being set in the general case, and the Beyonce rule appears to indicate that nobody likes this behaviour (as nobody put a test on it) I think it's safe to say we should just not modify the ctx, and constify it.
For #28654
Checklist