httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jean-Jacques Clar" <JJC...@novell.com>
Subject Re: [PATCH2] Re: Seg fault: race conditions in mod_mem_cache.c
Date Wed, 15 Sep 2004 15:53:27 GMT
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

>>> gregames@remulak.net 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



Mime
View raw message