Re: [PATCH2] Re: Seg fault: race conditions in mod_mem_cache.c

[Jean-Jacques Clar]

What about calling memcache_cache_free() instead of the apr_atomic_set(). The refcount must be more than 1, we hold the mutex. It should work just fine.. What do you think? JJ >>> [EMAIL PROTECTED] 09/15/04 8:26 AM >>> Jean-Jacques Clar wrote: > Should I then go ahead and commit my patch to the 2.1 tree? This section from decrement_refcount() makes me nervous: -        if (!obj->cleanup) { +        if (!(apr_atomic_read(&obj->refcount) & OBJECT_CLEANUP_BIT)) {               cache_remove(sconf->cache_cache, obj); -            obj->cleanup = 1; +            apr_atomic_set(&obj->refcount, obj->refcount | OBJECT_CLEANUP_BIT);   }           if (sconf->lock) {               apr_thread_mutex_unlock(sconf->lock); @@ -288,10 +288,8 @@       }       /* Cleanup the cache object */ -    if (!apr_atomic_dec(&obj->refcount)) { -        if (obj->cleanup) { -            cleanup_cache_object(obj); -        } +    if(apr_atomic_dec(&obj->refcount) == OBJECT_CLEANUP_BIT) { +        cleanup_cache_object(obj);       } It's the use of apr_atomic_set().  That line will take a snapshot of the value of obj->refcount at some point in time, OR on the OBJECT_CLEANUP_BIT, then unconditionally store the result back into obj->refcount. But there could be an unlocked caller trying to simultaneously decrement the refcount on another thread, right?  I say that because of the use of apr_atomic_dec() a few lines later.  If so, the apr_atomic_set could loose the decrement from the other thread. It would be better to use apr_atomic_cas() to set the cleanup bit because we can retry if the refcount changes. Greg