Fix memory leaks in CA related functions. by PavelKopyl · Pull Request #4700 · openssl/openssl

PavelKopyl · 2017-11-08T14:31:59Z

Fix memory leaks in CA related functions.
CLA: trivial

Checklist

documentation is added or updated
tests are added or updated

mattcaswell · 2017-11-08T14:34:02Z

@PavelKopyl do you think you could submit a CLA please? I think we are starting to stray beyond the "trivial" exception.

PavelKopyl · 2017-11-08T14:56:51Z

Ok, I'll start an approval to sign the CLA, but it may take some time.

mattcaswell · 2017-11-08T14:59:44Z

If this is a corporate contribution then you will need to send both a CCLA from your company and an ICLA for yourself.

mattcaswell

Looks good. Which branches does this apply to aside from master? 1.1.0? 1.0.2?

mattcaswell · 2018-01-24T15:28:49Z

apps/ca.c

Shouldn't we also free x here too?

Yes, you're right.

Some changes are required to apply it to 1.1.0 and 1.0.2. I'll make appropriate pull requests.

richsalz · 2018-03-07T20:48:51Z

crypto/engine/eng_lib.c

    item = int_cleanup_item(cb);
-    if (item)
-        sk_ENGINE_CLEANUP_ITEM_push(cleanup_stack, item);
+    if (item) {


Should that be tested against NULL? Not worth delaying this PR if it will take awhile to get around to fixing that nit.

mattcaswell · 2018-03-21T11:45:22Z

Looks like this is in need of a rebase.

levitte · 2018-04-19T13:01:03Z

The conflict in apps/ca.c is this:

<<<<<<< HEAD
    if (ok != 1) {
=======
    if (irow != NULL || ok <= 0) {
>>>>>>> Fix memory leaks in CA related functions.

As far as I can see, we can simply drop the apps/ca.c change and use the rest. If a merge can be approved under those conditions, I can do it.

richsalz

You'll keep the first ca.c change, right?
And can you fix my "test against NULL" nit?
Approved either way.

Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #4700)

levitte · 2018-05-02T06:39:43Z

Merged into master / 1.1.1, with the test against NULL nit fixed.

aebd0e5 Fix memory leaks in CA related functions.

levitte · 2018-05-02T06:43:30Z

This doesn't cherry-pick cleanly to 1.1.0, although that's only because of a small nit, 1.1.0 has an assertion that isn't in 1.1.1:

diff --cc crypto/conf/conf_api.c
index 5535416ab3,9606b7f222..0000000000
--- a/crypto/conf/conf_api.c
+++ b/crypto/conf/conf_api.c
@@@ -204,7 -204,8 +204,12 @@@ CONF_VALUE *_CONF_new_section(CONF *con
      v->value = (char *)sk;
  
      vv = lh_CONF_VALUE_insert(conf->data, v);
++<<<<<<< HEAD
 +    OPENSSL_assert(vv == NULL);
++=======
+     if (vv != NULL || lh_CONF_VALUE_error(conf->data) > 0)
+         goto err;
++>>>>>>> aebd0e5ca1... Fix memory leaks in CA related functions.
      return v;
  
   err:

Porting this to 1.0.2 is more extensive.

mspncp · 2018-05-12T22:23:54Z

@levitte, this pr was merged to master in aebd0e5. Can it be closed now, or do you still intend to do some backporting?

levitte · 2018-05-12T22:44:20Z

See #6237 and #6238

levitte · 2018-06-21T08:45:24Z

The backports are merged, closing this.

mattcaswell added the hold: cla required The contributor needs to submit a license agreement label Nov 9, 2017

PavelKopyl force-pushed the fix_memory_leaks branch from 10faed9 to cbbe030 Compare January 16, 2018 08:08

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 16, 2018

mattcaswell requested changes Jan 24, 2018

View reviewed changes

mattcaswell added this to the 1.1.1 milestone Jan 24, 2018

Fix memory leaks in CA related functions.

29b1795

PavelKopyl force-pushed the fix_memory_leaks branch from cbbe030 to 29b1795 Compare January 28, 2018 20:14

richsalz approved these changes Mar 7, 2018

View reviewed changes

richsalz approved these changes Apr 19, 2018

View reviewed changes

levitte pushed a commit that referenced this pull request May 2, 2018

Fix memory leaks in CA related functions.

aebd0e5

Reviewed-by: Rich Salz <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #4700)

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

This was referenced May 12, 2018

Fix memory leaks in CA related functions. [1.1.0] #6237

Closed

Fix memory leaks in CA related functions. [1.0.2] #6238

Closed

levitte closed this Jun 21, 2018

Uh oh!

Comments

Conversation

PavelKopyl commented Nov 8, 2017

Checklist

Uh oh!

mattcaswell commented Nov 8, 2017

Uh oh!

PavelKopyl commented Nov 8, 2017

Uh oh!

mattcaswell commented Nov 8, 2017

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

PavelKopyl Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

PavelKopyl Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

richsalz Mar 7, 2018

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Mar 21, 2018

Uh oh!

levitte commented Apr 19, 2018

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

levitte commented May 2, 2018

Uh oh!

levitte commented May 2, 2018

Uh oh!

mspncp commented May 12, 2018

Uh oh!

levitte commented May 12, 2018

Uh oh!

levitte commented Jun 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants