Sander Striker wrote:
Sander Striker wrote:
[...]
Coupling the registration to thread creation, rather than SMS creation, resolves some of the concerns that I listed previously. But I still have some questions about what happens during alloc/free in this model. Is your plan that the alloc/free functions should check sms->thread_count and do locking if it is greater than 1?
No, when the thread_count is increased, the lock is automatically created in the apr_sms_thread_register function.
In the code we do this:
if (SMS_XXX_T(sms)->lock) apr_lock_acquire(SMS_XXX_T(sms)->lock);
I still count two, or maybe three, race conditions here: * The lock can become non-null between the if-statement and the allocation code that follows (because somebody registers with the SMS from another thread). * In apr_sms_thread_register, two threads can collide in this part: if (!sms->lock) { sms_lock = apr_lock_create(); } It's possible for a newly created lock to be leaked in this code. * I'm not sure if the pointer assignment "sms_lock = ..." is going to be atomic on all the supported architectures.
[...]
The child thread is register in the parent thread. So, in the case of the first created thread, a lock will be created. This does not happen in the stub.
That eliminates the first race condition, but not the second. Consider this usage: * There are two global SMSs, sms1 and sms2. * Two threads, p1 and p2, are created with sms1 as the SMS supplied to apr_thread_create. * Prior to the creation of p1 and p2, sms2 is unused. * p1 creates a child thread, c1, and passes sms2 as the SMS for apr_thread_create to use. * p2 creates a child thread, c2, and passes sms2 as the SMS for apr_thread_create to use.
During the last two steps, we're vulnerable to the lock creation race condition.
As far as I know, this isn't a case that will ever happen in Apache, but it's possible in APR based apps in general. I guess there are two ways to solve it: 1) add code to defend against the race condition, or 2) impose a convention that says that p1 and p2 must register themselves with sms2 before they're allowed to create any other threads that use it.
--Brian
