httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: Memory leak in mod_ssl ssl_callback_TmpDH
Date Tue, 27 May 2014 20:33:25 GMT


Joe Orton wrote:
> On Sat, May 24, 2014 at 10:32:35AM +0200, Kaspar Brand wrote:
>> On 19.05.2014 10:15, Plüm, Rüdiger, Vodafone Group wrote:
>>> Maybe stupid idea, but can't we do that once and hand it out over
>>> and over again?
>>
>> Not a stupid idea at all - I think it's actually the most sensible
>> solution to this problem. This is what OpenSSL does with the
>> DH parameters provided by the callback in s3_srvr.c:ssl3_send_server_key_exchange():
> 
> This may be a stupid question: if we are doing this once per process 
> lifetime would it not be better to do it at init time, and store the 
> results somewhere other than a static variable?
> 
> We have a potential race here between threads doing the param 
> generation, right?
> 
> +    static DH *dh = NULL; \
> +    DH *dh_tmp; \
> ...
> +    dh = dh_tmp; \
> 
> though it would not matter who wins the race *if* we could rely on 
> pointer assignment being atomic - which is a fairly dubious assumption, 

That was my assumption. If we cannot assume that the pointer assignment is atomic,
then we have a race problem.

Would the following patch address your concern?

Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c     (revision 1597665)
+++ modules/ssl/ssl_engine_kernel.c     (working copy)
@@ -31,6 +31,7 @@
 #include "ssl_private.h"
 #include "mod_ssl.h"
 #include "util_md5.h"
+#include "apr_atomic.h"

 static void ssl_configure_env(request_rec *r, SSLConnRec *sslconn);
 #ifdef HAVE_TLSEXT
@@ -1320,6 +1321,10 @@
  * contrast to the keys itself) and code safe as the returned structure
  * is duplicated by OpenSSL anyway. Hence no modification happens
  * to our copy.
+ *
+ * Use apr_atomic_xchgptr to assign the result to dh as we might have
+ * multiple threads calling us in parallel and pointer assignment might
+ * not be atomic.
  */
 #define make_get_dh(rfc,size,gen) \
 static DH *get_dh##size(void) \
@@ -1339,7 +1344,7 @@
         DH_free(dh_tmp); \
         return NULL; \
     } \
-    dh = dh_tmp; \
+    apr_atomic_xchgptr((volatile void **)&dh, dh_tmp); \
     return dh; \
 }

> and at least deserves a comment.  If a potential race is possible here 
> it might be better to do it once at startup to save CPU time anyway?

I am not that worried about CPU because I guess we fairly quickly get dh set to a valid value
and afterwards stuff is
fast, but for sure this would be another option.

Regards

Rüdiger


Mime
View raw message