Skip to content
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

PKCS12_SAFEBAG_set0_attrs: Remove const from function signature #19359

Closed
wants to merge 2 commits into from

Conversation

faramir-dev
Copy link
Contributor

@faramir-dev faramir-dev commented Oct 7, 2022

Checklist
  • documentation is added or updated
  • tests are added or updated

@@ -125,5 +125,5 @@ void PKCS12_SAFEBAG_set0_attrs(PKCS12_SAFEBAG *bag, const STACK_OF(X509_ATTRIBUT
if (bag->attrib != attrs)
sk_X509_ATTRIBUTE_free(bag->attrib);

bag->attrib = (STACK_OF(X509_ATTRIBUTE*))attrs;
bag->attrib = (STACK_OF(X509_ATTRIBUTE) *)attrs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. - this function doesn't quite look right. The cast is only necessary here because we are casting away the "const". But it probably should not be const at all, because we eventually plan to "free" this value. So I think the correct fix here is to remove the cast altogether and adjust the function signature to remove "const". This is ok because this function is newly added to master and doesn't exist in other branches - so backwards compatibility is not a concern.

@faramir-dev faramir-dev changed the title Fix typo in PKCS12_SAFEBAG_set0_attrs PKCS12_SAFEBAG_set0_attrs: Remove const from function signature Oct 7, 2022
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Oct 7, 2022
@t8m t8m added approval: done This pull request has the required number of approvals triaged: bug The issue/pr is/fixes a bug and removed approval: review pending This pull request needs review by a committer labels Oct 7, 2022
@openssl openssl deleted a comment Oct 8, 2022
@openssl openssl deleted a comment Oct 8, 2022
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 8, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member

hlandau commented Oct 13, 2022

Merged to master. Thank you.

@hlandau hlandau closed this Oct 13, 2022
openssl-machine pushed a commit that referenced this pull request Oct 13, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #19359)
openssl-machine pushed a commit that referenced this pull request Oct 13, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from #19359)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#19359)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from openssl#19359)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants