Fixes #20145: free memory of certStatus before goto err #20406

mhjd · 2023-02-28T20:31:08Z

I have fix the issue #20145 with freeing memory of certStatus and changed the condition as asked in the issue.
I changed :

/* consume certStatus into msg right away so it gets deallocated with msg */ 
if (!sk_OSSL_CMP_CERTSTATUS_push(msg->body->value.certConf, certStatus)) 
    goto err;

Into :

/* consume certStatus into msg right away so it gets deallocated with msg */ 
if (sk_OSSL_CMP_CERTSTATUS_push(msg->body->value.certConf, certStatus) < 1) {
/*
 * It hasn't been consumed by the msg at this point of time,
 * so we should free certStatus */
    OSSL_CMP_CERTSTATUS_free(certStatus);
    goto err;
}

crypto/cmp/cmp_msg.c

slontis · 2023-02-28T23:28:56Z

crypto/cmp/cmp_msg.c

@@ -817,8 +817,13 @@ OSSL_CMP_MSG *ossl_cmp_certConf_new(OSSL_CMP_CTX *ctx, int fail_info,
    if ((certStatus = OSSL_CMP_CERTSTATUS_new()) == NULL)
        goto err;
    /* consume certStatus into msg right away so it gets deallocated with msg */
-    if (!sk_OSSL_CMP_CERTSTATUS_push(msg->body->value.certConf, certStatus))
+    if (sk_OSSL_CMP_CERTSTATUS_push(msg->body->value.certConf, certStatus) < 1) {


@DDvO What frees this stack in the normal case when this passes?

As usual with ASN.1 structures, on success ownership of field contents is passed to the containing structure, so the (stack of) OSSL_CMP_CERTSTATUS will be freed along with the msg containing it.
I even tried to make this (rather) explicit here by the comment above.

DDvO

Please just remove the needless comment,
and apart from this the two fixes done here here are good.

DDvO · 2023-03-01T08:00:26Z

Can we assert CLA: trivial,
like done recently for #20351?

DDvO · 2023-03-01T08:02:40Z

As these fixes should be done for all versions since 3.0, I added the respective labels.

DDvO · 2023-03-01T10:15:23Z

Can somebody explain to me why suddenly

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label 2 hours ago

? So far I had assumed that this label has to do mainly with API (public header file) changes.

mattcaswell · 2023-03-01T10:20:24Z

Can somebody explain to me why suddenly

Maybe the script is confused by the merge commits in this PR???

DDvO · 2023-03-01T10:26:02Z

Maybe the script is confused by the merge commits in this PR???

Good hint - with the but-last push, which did not actually change the actual PR contents
but maybe removed any confusing merge commits, the fips change label disappeared :)

DDvO

Fine, assuming the CLA topic will be sorted out before merging.

t8m · 2023-03-01T10:58:21Z

Please drop the merge commits by git rebase -i

t8m · 2023-03-01T10:58:49Z

I would be OK with CLA: trivial for this.

DDvO · 2023-03-02T10:39:17Z

In one of the CI runs, unix-macos11-m1, one of the test cases in 80-test_cmp_http.t failed with a timeout,
but I presume that this due to a timing issue on the CI machine and surely is unrelated.

DDvO · 2023-03-07T15:33:29Z

I would be OK with CLA: trivial for this.

Do we need more people to agree here, or who is entitled to remove the hold: cla required?

t8m

OK with CLA: trivial

mattcaswell · 2023-03-07T16:31:13Z

who is entitled to remove the hold: cla required?

All reviewers and the author must agree that a contribution is trivial and then a committer is allowed to lift it.

DDvO · 2023-03-07T16:53:51Z

@mhjd do you agree that the contribution is simple enough for not having to submit a CLA?
Please see the details on https://www.openssl.org/policies/cla.html and follow the instructions there.

mhjd · 2023-03-07T18:59:15Z

@mhjd do you agree that the contribution is simple enough for not having to submit a CLA? Please see the details on https://www.openssl.org/policies/cla.html and follow the instructions there.

I agree, it's trivial, no need to submit a CLA 👌

t8m · 2023-03-08T07:43:18Z

I agree, it's trivial, no need to submit a CLA ok_hand

@mhjd Could you please use git rebase -i to drop the merge commits from the PR?

openssl-machine · 2023-03-08T16:00:21Z

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.

mhjd · 2023-03-09T18:31:42Z

I agree, it's trivial, no need to submit a CLA ok_hand

@mhjd Could you please use git rebase -i to drop the merge commits from the PR?

I wait the help of my professor, I have some problem with git rebase because it drop too many commits. Nevertheless I plan to retry when I have time, tomorrow or Saturday.

DDvO · 2023-03-20T12:19:06Z

I doubt this PR can really be merged in the current state, given the merge commits included.

mhjd · 2023-03-20T16:23:42Z

Sorry for the delay, I intend to try to solve it today. (There's been a strike problem for the last few weeks so I can't ask my teacher for help).

t8m · 2023-03-20T18:00:02Z

We can resolve the problem when merging.

CLA: trivial Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20406)

CLA: trivial Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20406) (cherry picked from commit c9c9901)

t8m · 2023-03-20T18:09:26Z

Merged to master, 3.1, and 3.0 branches after squashing the commits. Thank you for your contribution.

Fixes openssl#20145: free memory of certStatus before goto err

2c1d023

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Feb 28, 2023

slontis reviewed Feb 28, 2023

View reviewed changes

crypto/cmp/cmp_msg.c Outdated Show resolved Hide resolved

slontis reviewed Feb 28, 2023

View reviewed changes

JAVAID Mohammad-Habib and others added 3 commits March 1, 2023 07:36

Fixes openssl#20145: free memory of certStatus before goto err

5236576

Merge branch 'openssl:master' into master

d9a32b5

Fixes openssl#20145: commentary fixed

06ed44a

DDvO requested changes Mar 1, 2023

View reviewed changes

DDvO added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 labels Mar 1, 2023

DDvO added the triaged: bug The issue/pr is/fixes a bug label Mar 1, 2023

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Mar 1, 2023

Merge branch 'openssl:master' into master

5db3d78

github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Mar 1, 2023

Fixes openssl#20145: remove the needless comment

213f36f

DDvO approved these changes Mar 1, 2023

View reviewed changes

DDvO removed the approval: review pending This pull request needs review by a committer label Mar 1, 2023

t8m approved these changes Mar 7, 2023

View reviewed changes

paulidale 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 Mar 14, 2023

openssl-machine pushed a commit that referenced this pull request Mar 20, 2023

cmp_msg.c: free memory of certStatus before goto err

c9c9901

CLA: trivial Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #20406)

t8m mentioned this pull request Mar 20, 2023

Partial error value check & possible memory leak in crypto/cmp/cmp_msg.c #20145

Closed

t8m closed this Mar 20, 2023

Uh oh!

Fixes #20145: free memory of certStatus before goto err #20406

Fixes #20145: free memory of certStatus before goto err #20406

Uh oh!

Conversation

mhjd commented Feb 28, 2023

Uh oh!

Uh oh!

slontis Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

DDvO commented Mar 1, 2023

Uh oh!

DDvO commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO commented Mar 1, 2023

Uh oh!

mattcaswell commented Mar 1, 2023

Uh oh!

DDvO commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DDvO left a comment

Choose a reason for hiding this comment

Uh oh!

t8m commented Mar 1, 2023

Uh oh!

t8m commented Mar 1, 2023

Uh oh!

DDvO commented Mar 2, 2023

Uh oh!

DDvO commented Mar 7, 2023

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Mar 7, 2023

Uh oh!

DDvO commented Mar 7, 2023

Uh oh!

mhjd commented Mar 7, 2023

Uh oh!

t8m commented Mar 8, 2023

Uh oh!

openssl-machine commented Mar 8, 2023

Uh oh!

mhjd commented Mar 9, 2023

Uh oh!

DDvO commented Mar 20, 2023

Uh oh!

mhjd commented Mar 20, 2023

Uh oh!

t8m commented Mar 20, 2023

Uh oh!

t8m commented Mar 20, 2023

Uh oh!

Uh oh!

slontis Feb 28, 2023 •

edited

Loading

DDvO Mar 1, 2023 •

edited

Loading

DDvO commented Mar 1, 2023 •

edited

Loading

DDvO commented Mar 1, 2023 •

edited

Loading