David Schwartz wrote:
1287 void SSL_free_comp_methods(void)
1288 {
1289 if (ssl_comp_methods == NULL)
1290 return;
1291 CRYPTO_w_lock(CRYPTO_LOCK_SSL);
1292 if (ssl_comp_methods != NULL)
1293 {
1294 sk_SSL_COMP_pop_free(ssl_comp_methods,CRYPTO_free);
1295 ssl_comp_methods = NULL;
1296 }
1297 CRYPTO_w_unlock(CRYPTO_LOCK_SSL);
1298 }
For POSIX threads, the result of reading a variable in one thread while it
might be modified in another thread is undefined. Line 1289 and 1290 should
be removed.
Not this old chestnut again.
I can't name a CPU in which an aligned load/store of a natural/primitive
data size between overlapping threads actually has "undefined" behavior.
The underlying assembly instructions generated by the C compiler have
a defined behavior on multi-CPU systems.
Due to this defined behavior of the underlying CPU at a lower level than
the scope of the POSIX specification is designed to cover you should
treat this behavior as defined. This would not contradict POSIX which
is effectively saying "out of the scope of the POSIX spec" by using the
term "undefined".
The function entrance, the "return" and the "CRYPTO_w_lock()" calls all
act as the correct memory barriers with regards to compiler
optimizations and load/store rescheduling concerns. Even if the
CRYPTO_w_lock() is a macro or inline, since the writer would have been
sure to insert the correct barriers if he chose to implement
CRYPTO_w_lock() as a macro/inline.
Alternatively, lines 1288, 1289, 1290, and 1297 should be removed. It's hard
to understand any reasonable scenario in which one thread might be calling
SSL_free_comp_methods while another thread is or might be accessing the
ssl_comp_methods variable or any of the registered compression methods.
I would agree with your overall view, but I would on the basis that this
function is not performance critical to deserve any special lock
contention avoidance treatment.
But.... there is a case for locking to not be necessary at all. If the
SSL_free_comp_methods() is only designed to be used during the cleanup
phase. During a clean shutdown sequence of the OpenSSL library
I'm pretty sure you are meant to have reverted the OpenSSL call
execution path to single threaded mode.
Meaning while you are executing any cleanup functions it is not safe to
be calling any other OpenSSL functions at the same time. By virtue of
this requirement locking should not be necessary (but should do more
good than harm if implemented). If your application needs to repeatedly
init and cleanup, init and cleanup, then your application needs to also
manage the state and the serialization of the init/cleanup calls (since
they may not be safe to be called out-of-order, called twice,
interleved, etc...).
But the code as posted is an unreasonable compromise between two reasonable
positions. It is "sort of" safe if called while another thread is or might
be accesing ssl_comp_methods.
This I'm not so sure I agree with; the "It is 'sort of' safe if called
while another thread ..." part (see the last 2 paragraphs of mine above).
If you think there is no way this code could fail, imagine a hypothetical
compiler/platform in which "if(ssl_comp_methods == NULL)" translates into an
operation through a fast caching shadow register like this:
shadow=ssl_comp_methods;
if(ssl_comp_methods==NULL)
{
ssl_comp_methods=shadow; /* flush cache, writing back entries */
/* whatever */
}
If another thread modified 'ssl_comp_methods' in between the read and the
cache flush, that modification would be lost. I, for one, don't like
needlessly making code potentially disastrous on future CPUs and compilers.
(To save what?!)
... the evils of premature optimization.
No as I said by virtue of it being a function it already forces the
compiler to perform the correct barriers. Maybe you can name a CPU
target which does not automatically flush pending writes for "return
from function" and "call a function" instructions, I can't.
The example you cite is not valid for line 1289 and 1290. The only
write operation to 'ssl_comp_methods' occurs inside the locked region
and after the text condition has been re-evaluated.
So your pseudo-code serves a useful purpose to illustrate to others on
the list about the concern you raise, but the concern itself is not
relevant to the example given.
Darryl
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager [EMAIL PROTECTED]
