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.

Sign up for GitHub

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

Jump to bottom

statem_srvr.c : Release pkey_ctx on initialization failure #24366

Closed
wants to merge 1 commit into from
Closed

statem_srvr.c : Release pkey_ctx on initialization failure #24366

wants to merge 1 commit into from

Conversation

irosay
Copy link
Contributor

@irosay irosay commented May 10, 2024

Fixes #24341

Checklist

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 10, 2024
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 labels May 13, 2024
@t8m
Copy link
Member

t8m commented May 13, 2024

@irosay This would be ok with CLA: trivial. Could you please amend the commit message with git commit --amend; git push --force to include CLA: trivial in the commit message body on a separate line?

@irosay irosay force-pushed the master branch 2 times, most recently from 675a0ef to 033dd55 Compare May 13, 2024 12:00
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label May 13, 2024
@irosay irosay marked this pull request as ready for review May 13, 2024 12:06
mattcaswell
mattcaswell requested changes May 13, 2024
View reviewed changes
ssl/statem/statem_srvr.c Outdated
@@ -3229,6 +3229,7 @@ static int tls_process_cke_gost(SSL_CONNECTION *s, PACKET *pkt)
return 0;
}
if (EVP_PKEY_decrypt_init(pkey_ctx) <= 0) {
EVP_PKEY_CTX_free(pkey_ctx);
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

A fix more consistent with the rest of this function might be to instead replace this return 0 with a goto err

@irosay irosay requested a review from mattcaswell May 13, 2024 12:45
mattcaswell
mattcaswell requested changes May 13, 2024
View reviewed changes
ssl/statem/statem_srvr.c Outdated
@@ -3229,8 +3229,9 @@ static int tls_process_cke_gost(SSL_CONNECTION *s, PACKET *pkt)
return 0;
}
if (EVP_PKEY_decrypt_init(pkey_ctx) <= 0) {
EVP_PKEY_CTX_free(pkey_ctx);
Copy link
Member

Choose a reason for hiding this comment

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

This now isn't necessary since the err block already does it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay thank you

@irosay irosay requested a review from mattcaswell May 13, 2024 13:15
@mattcaswell mattcaswell added cla: trivial One of the commits is marked as 'CLA: trivial' and removed approval: otc review pending This pull request needs review by an OTC member labels May 13, 2024
@mattcaswell
Copy link
Member

Ok with trivial

@t8m
Copy link
Member

t8m commented May 13, 2024

OK with CLA: trivial

@t8m t8m removed the approval: review pending This pull request needs review by a committer label May 13, 2024
@t8m t8m added the approval: done This pull request has the required number of approvals label May 13, 2024
@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 May 14, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented May 14, 2024

Merged to all the active branches. Thank you for your contribution.

@t8m t8m closed this May 14, 2024
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@irosay @t8m
Release pkey_ctx on initialization failure
f7cbc2d 
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24366)

(cherry picked from commit 3e9d933)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@irosay @t8m
Release pkey_ctx on initialization failure
c4ecb00 
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24366)

(cherry picked from commit 3e9d933)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@irosay @t8m
Release pkey_ctx on initialization failure
62b684a 
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24366)

(cherry picked from commit 3e9d933)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@irosay @t8m
Release pkey_ctx on initialization failure
4cbbb20 
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24366)

(cherry picked from commit 3e9d933)
openssl-machine pushed a commit that referenced this pull request May 14, 2024
@irosay @t8m
Release pkey_ctx on initialization failure
3e9d933 
CLA: trivial

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #24366)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 branch: 3.2 Merge to openssl-3.2 branch: 3.3 Merge to openssl-3.3 cla: trivial One of the commits is marked as 'CLA: trivial' tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

There is a memory leak defect at line 3232 of the file /openssl/ssl/statem/statem_srvr.c.
4 participants
@irosay @t8m @mattcaswell @openssl-machine