On Mon, Jul 09, 2001 at 09:44:29AM -0700, Travis Vitek wrote:
> This fix seems to work fine, but there is still a (small) potential
> problem...
>
> Consider the situation where thread id 0 is valid (this is true for some
> systems). If one thread (thread id N) unsets the flags and releases the lock
> (ssleay_rand_bytes)
>
> crypto_lock_rand = 0;
> locking_thread = 0;
> CRYPTO_w_unlock(CRYPTO_LOCK_RAND);
>
> Another thread (thread id M) locks and starts to set the flags
> (ssleay_rand_bytes)
>
> CRYPTO_w_lock(CRYPTO_LOCK_RAND);
> crypto_lock_rand = 1;
>
> Finally, thread 0 does a check to see if it should lock or not...
> (ssleay_rand_add)
>
> do_not_lock = cypto_lock_rand && (locking_thread == CRYPTO_thread_id());
>
> it will not lock as it should, because it thinks it owns the lock, which is
> actually owned by thread M... Not good!
You are absolutely correct. Actually I was aware that 0 can be a
valid thread ID (this is one reason for using the 'crypto_lock_rand'
flag in addition to the 'locking_thread' variable, similar to a change
I did in crypto/mem_dbg.c earlier on), but obviously I missed this
particular race condition. Thanks for sharing your observation.
> I believe that the fix is fairly simple...
>
> 1. When setting 'crypto_lock_rand = 1', only do so after setting
> 'locking_thread = CRYPTO_thread_id()'... This will guarantee that a thread
> checking do_not_lock will not see crypto_lock_rand flag set until after the
> true locking_thread has been set.
>
> // md_rand.c:355 and md_rand.c:520
> CRYPTO_w_lock(CRYPTO_LOCK_RAND);
>
> crypto_lock_rand = 1;
> locking_thread = CRYPTO_thread_id();
>
> would be written like this...
>
> CRYPTO_w_lock(CRYPTO_LOCK_RAND);
>
> locking_thread = CRYPTO_thread_id();
> crypto_lock_rand = 1;
This was also my guess when reading your above problem description.
Hopefully this is a sign that this time it is really correct.
Actually it can not be correct if access to 'unsigned long' values is
not atomic. If I decide to be that pedantic (and I start to feel I
should), I have to introduce an additional lock for protecting access
to 'locking_thread'. This does not add significant overhead as
this is only needed on the few occasions when 'crypto_lock_rand' is
set.
> 2. When setting 'crypto_lock_rand = 0', just leave locking_thread alone.
> This just removes an unnecessary assignment
True, 'crypto_lock_rand = 42' would have made just as much sense.
Well, at least this is technically not a bug (but it isn't good
programming style either).
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List [EMAIL PROTECTED]
Automated List Manager [EMAIL PROTECTED]
