Patrick McCormick wrote, > I don't think you have pulled the latest version of this patch from > CVS. Bodo Moeller and I went through a few iterations of this patch, > and the final version performs double-init checking where necessary. > (The discussion should be in the mailing list archives.) I checked > the above code just now, and on the OpenSSL_0_9_6-stable branch it is > different and correct.
Yup, it now does the cannonical double-check. But ... > Please review the final patch and notify the list if you find > problems. I would feel better about asking the team to cut 0.9.6h > after some other people have reviewed the changes. ... that's not guaranteed to fix the problem on all architectures. See, http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html Ignore the fact that it's mostly to do with Java, and scroll down to the section headed "Making it work with explicit memory barriers". There's an extended discussion of the issues, esp. for MP Alpha machines in [1]. First, the init flag needs to be volatile to prevent an aggressive compiler from optimizing away the second check (nb. there may be issues with compilers respecting volatile as mentioned in the key zeroizing thread). Second you need to deal with MP cache coherency (Alpha, Itanium, others?). Memory models can be so weak that reads and writes to the same address can be reordered, and a write from a thread on one CPU isn't guaranteed to be immediately visible to a subsequent read from a thread on another. Putting those two together, I think we end up with something like, SSL_METHOD *SSLv3_client_method(void) { static int init=1; static SSL_METHOD SSLv3_client_data; volatile int tmp = init; #if defined (ALPHA_MP) // CPU-specific memory barrier instruction asm("mb"); #endif if (tmp) { CRYPTO_w_lock(CRYPTO_LOCK_SSL_METHOD); if (tmp) { memcpy((char *)&SSLv3_client_data,(char *)sslv3_base_method(), sizeof(SSL_METHOD)); SSLv3_client_data.ssl_connect=ssl3_connect; SSLv3_client_data.get_ssl_method=ssl3_get_client_method; #if defined (ALPHA_MP) // CPU-specific memory barrier instruction asm("mb"); #endif init=0; } CRYPTO_w_unlock(CRYPTO_LOCK_SSL_METHOD); } return(&SSLv3_client_data); } (nb. adapted from [1], so don't shoot me if I haven't got it exactly right) Unfortunately there are two big problems with this: we've got CPU and compiler specific tweaks, which don't exactly help portability; and using volatile and explicit memory barriers even for the fast, already initialized, path is a performance hit. It might be better to avoid lazy initialization altogether or move the whole init sequence into the critical section, eg., SSL_METHOD *SSLv3_client_method(void) { static int init=1; static SSL_METHOD SSLv3_client_data; CRYPTO_w_lock(CRYPTO_LOCK_SSL_METHOD); if (init) { memcpy((char *)&SSLv3_client_data,(char *)sslv3_base_method(), sizeof(SSL_METHOD)); SSLv3_client_data.ssl_connect=ssl3_connect; SSLv3_client_data.get_ssl_method=ssl3_get_client_method; init=0; } CRYPTO_w_unlock(CRYPTO_LOCK_SSL_METHOD); return(&SSLv3_client_data); } You'll take a performance hit, but at least this is portably race-free. Cheers, Miles [1] Schmidt, Stal, Rohnert and Buschmann, Pattern-Oriented Software Architecture, vol. 2, Wiley 2000, pp. 353-367 ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [EMAIL PROTECTED] Automated List Manager [EMAIL PROTECTED]
