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
