Re: Memory leak in mod_ssl ssl_callback_TmpDH

[Ruediger Pluem]

Joe Orton wrote:
> On Wed, May 28, 2014 at 01:58:05PM +0000, Plüm, Rüdiger, Vodafone Group wrote:
>> Looks great. Care to commit?
> 
> http://svn.apache.org/viewvc?view=revision&revision=1598107
> 
> 

Thanks, but I missed some stuff during review:

1. We don't need to have two DH pointers in make_dh_params
2. There possible frees on NULL pointers in free_dh_params:

The following patch should fix this:

Index: modules/ssl/ssl_engine_init.c

===================================================================

--- modules/ssl/ssl_engine_init.c       (revision 1598117)

+++ modules/ssl/ssl_engine_init.c       (working copy)

@@ -54,21 +54,16 @@

 static DH *make_dh_params(BIGNUM *(*prime)(BIGNUM *), const char *gen)

 {
     DH *dh = NULL;
-    DH *dh_tmp;

-    if (dh) {
-        return dh;
-    }
-    if (!(dh_tmp = DH_new())) {
+    if (!(dh = DH_new())) {
         return NULL;
     }
-    dh_tmp->p = prime(NULL);
-    BN_dec2bn(&dh_tmp->g, gen);
-    if (!dh_tmp->p || !dh_tmp->g) {
-        DH_free(dh_tmp);
+    dh->p = prime(NULL);
+    BN_dec2bn(&dh->g, gen);
+    if (!dh->p || !dh->g) {
+        DH_free(dh);
         return NULL;
     }
-    dh = dh_tmp;
     return dh;
 }

@@ -87,10 +82,18 @@

 static void free_dh_params(void)
 {
-    DH_free(dhparam1024);
-    DH_free(dhparam2048);
-    DH_free(dhparam3072);
-    DH_free(dhparam4096);
+    if (dhparam1024) {
+        DH_free(dhparam1024);
+    }
+    if (dhparam2048) {
+        DH_free(dhparam2048);
+    }
+    if (dhparam3072) {
+        DH_free(dhparam3072);
+    }
+    if (dhparam4096) {
+        DH_free(dhparam4096);
+    }
     dhparam1024 = dhparam2048 = dhparam3072 = dhparam4096 = NULL;
 }



Objections or should I commit?

Regards

Rüdiger