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

Fix EVP_PKEY_asn1_copy #21125

Closed

Conversation

HangzeGao
Copy link

Add the copy of the omitted ASN1 public key method

Fixes #21115

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 5, 2023
@t8m t8m added branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch triaged: bug The issue/pr is/fixes a bug labels Jun 5, 2023
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 5, 2023
@paulidale paulidale added the approval: review pending This pull request needs review by a committer label Jun 6, 2023
@t8m t8m added tests: exempted The PR is exempt from requirements for testing cla: trivial One of the commits is marked as 'CLA: trivial' labels Jun 6, 2023
@t8m
Copy link
Member

t8m commented Jun 6, 2023

This is borderline on what we accept with CLA: trivial

@paulidale ?

@paulidale
Copy link
Contributor

I'm fine with trivial. There isn't another way to do this.

@tmshort
Copy link
Contributor

tmshort commented Jun 11, 2023

I don't see sig_print being copied?

There isn't another way to do this.

One could play with structure offsets and memcpy(); or by saving the dst real data (5 items), then memcpy(dst, src, sizeof(*src)), then restoring the dst data. An optimization would be to have the function and non-function data in different structs that could be copied separately.

Unless the compiler is smart enough to know the out-of-order listing (e.g. old_priv_decode) is contiguous memory, but since sig_print() is also missing, it can't.

@slontis
Copy link
Member

slontis commented Jun 12, 2023

In master we are doing:

 int  x = dst->x;
 ....
 *dst = *src
 
 dst->x = x;
 ....

Doing the change listed here is probably not trivial..

@tmshort
Copy link
Contributor

tmshort commented Jun 13, 2023

Doing the change listed here is probably not trivial..

For what I suggest, agreed.

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@tom-cosgrove-arm
Copy link
Contributor

@HangzeGao Can you update with @bernd-edlinger's suggestion, please?

@openssl/otc So if this is not acceptable under CLA: trivial, have we asked for @HangzeGao to sign and submit an ICLA?

@mattcaswell
Copy link
Member

https://github.com/orgs/openssl/teams/otc So if this is not acceptable under CLA: trivial, have we asked for @HangzeGao to sign and submit an ICLA?

We have reviewers saying it isn't trivial, so therefore a CLA is required. @HangzeGao - please can you submit one?

@HangzeGao HangzeGao closed this Aug 3, 2023
@HangzeGao HangzeGao reopened this Aug 3, 2023
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Aug 3, 2023
@HangzeGao HangzeGao closed this Aug 3, 2023
@HangzeGao HangzeGao reopened this Aug 3, 2023
Copy link
Member

@beldmit beldmit left a comment

Choose a reason for hiding this comment

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

LGTM

I'd consider it trivial, speaking frankly.

@beldmit beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Aug 3, 2023
@HangzeGao
Copy link
Author

https://github.com/orgs/openssl/teams/otc So if this is not acceptable under CLA: trivial, have we asked for @HangzeGao to sign and submit an ICLA?

We have reviewers saying it isn't trivial, so therefore a CLA is required. @HangzeGao - please can you submit one?

I have submitted the ICLA. I also closed and reopened it. But the CLA check still failed.

@mattcaswell
Copy link
Member

But the CLA check still failed.

It's failing because the email address in the author field of the commit does not match the email address provided in the CLA.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@bernd-edlinger
Copy link
Member

@HangzeGao please change the Author in your commit to your correct e-mail address like this:

git commit --amend --author "test <test@test.com>"
git push -f

Add the copy of the omitted ASN1 public key method
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Aug 13, 2023
@HangzeGao HangzeGao closed this Aug 16, 2023
@HangzeGao HangzeGao reopened this Aug 16, 2023
@t8m t8m removed the cla: trivial One of the commits is marked as 'CLA: trivial' label Aug 16, 2023
@t8m
Copy link
Member

t8m commented Aug 16, 2023

Merged to 1.1.1 branch. Thank you for your contribution.

@t8m t8m closed this Aug 16, 2023
openssl-machine pushed a commit that referenced this pull request Aug 16, 2023
Add the copy of the omitted ASN1 public key method and
other members.

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from #21125)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants