2330. [bug] Remove potential race condition when handling

                        over memory events. [RT #17572]

                        WARNING: API CHANGE: over memory callback
                        function now needs to call isc_mem_waterack().
                        See <isc/mem.h> for details.

diff --git a/CHANGES b/CHANGES
index 1f0cfaf337fad4f152e55dd228d678eb7c8c908f..4f7ace543a60c7a548620209c3232b923245768e 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,10 @@
+2330.  [bug]           Remove potential race condition when handling
+                       over memory events. [RT #17572]
+
+                       WARNING: API CHANGE: over memory callback
+                       function now needs to call isc_mem_waterack().
+                       See <isc/mem.h> for details.
+
 2329.  [bug]           Clearer help text for dig's '-x' and '-i' options.
 
 2328.  [bug]           Add AAAA addresses for A.ROOT-SERVERS.NET,

diff --git a/lib/dns/acache.c b/lib/dns/acache.c
index c95e2036639f8bfb96d04dd17cf53609a2c65be9..4472c4f90dcf3d45c651e187cb9329876fe0a60c 100644 (file)
--- a/lib/dns/acache.c
+++ b/lib/dns/acache.c
@@ -14,7 +14,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: acache.c,v 1.20 2007/06/19 23:47:16 tbox Exp $ */
+/* $Id: acache.c,v 1.21 2008/02/07 02:41:26 marka Exp $ */
 
 #include <config.h>
 
@@ -965,10 +965,14 @@ water(void *arg, int mark) {
 
        LOCK(&acache->cleaner.lock);
 
-       acache->cleaner.overmem = overmem;
+       if (acache->cleaner.overmem != overmem) {
+               acache->cleaner.overmem = overmem;
 
-       if (acache->cleaner.overmem_event != NULL)
-               isc_task_send(acache->task, &acache->cleaner.overmem_event);
+               if (acache->cleaner.overmem_event != NULL)
+                       isc_task_send(acache->task,
+                                     &acache->cleaner.overmem_event);
+               isc_mem_waterack(acache->mctx, mark);
+       }
 
        UNLOCK(&acache->cleaner.lock);
 }

diff --git a/lib/dns/adb.c b/lib/dns/adb.c
index 182f1c538685d359eb414d01d4ab1e43bb6adcdd..48cb9f988e20a790aed84b11378962e8f29acb0f 100644 (file)
--- a/lib/dns/adb.c
+++ b/lib/dns/adb.c
@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: adb.c,v 1.233 2007/10/19 17:15:53 explorer Exp $ */
+/* $Id: adb.c,v 1.234 2008/02/07 02:41:26 marka Exp $ */
 
 /*! \file
  *
@@ -128,6 +128,7 @@ struct dns_adb {
 
         isc_mutex_t                     lock;
         isc_mutex_t                     reflock; /*%< Covers irefcnt, erefcnt */
+        isc_mutex_t                     overmemlock; /*%< Covers overmem */
         isc_mem_t                      *mctx;
         dns_view_t                     *view;
         isc_timermgr_t                 *timermgr;
@@ -2138,6 +2139,7 @@ destroy(dns_adb_t *adb) {
         DESTROYLOCK(&adb->reflock);
         DESTROYLOCK(&adb->lock);
         DESTROYLOCK(&adb->mplock);
+        DESTROYLOCK(&adb->overmemlock);
 
         isc_mem_putanddetach(&adb->mctx, adb, sizeof(dns_adb_t));
 }
@@ -2225,6 +2227,10 @@ dns_adb_create(isc_mem_t *mem, dns_view_t *view, isc_timermgr_t *timermgr,
         if (result != ISC_R_SUCCESS)
                 goto fail0d;
 
+        result = isc_mutex_init(&adb->overmemlock);
+        if (result != ISC_R_SUCCESS)
+                goto fail0e;
+
         /*
          * Initialize the bucket locks for names and elements.
          * May as well initialize the list heads, too.
@@ -2343,6 +2349,8 @@ dns_adb_create(isc_mem_t *mem, dns_view_t *view, isc_timermgr_t *timermgr,
         if (adb->afmp != NULL)
                 isc_mempool_destroy(&adb->afmp);
 
+        DESTROYLOCK(&adb->overmemlock);
+ fail0e:
         DESTROYLOCK(&adb->reflock);
  fail0d:
         DESTROYLOCK(&adb->mplock);
@@ -3782,16 +3790,25 @@ water(void *arg, int mark) {
         DP(ISC_LOG_DEBUG(1),
            "adb reached %s water mark", overmem ? "high" : "low");
 
-        adb->overmem = overmem;
+       /*
+        * We can't use adb->lock as there is potential for water
+        * to be called when adb->lock is held.
+        */
+       LOCK(&adb->overmemlock);
+       if (adb->overmem != overmem) {
+               adb->overmem = overmem;
 #if 0       /* we don't need this timer for the new cleaning policy. */
-        if (overmem) {
-                isc_interval_t interval;
+               if (overmem) {
+                       isc_interval_t interval;
 
-                isc_interval_set(&interval, 0, 1);
-                (void)isc_timer_reset(adb->timer, isc_timertype_once, NULL,
-                                      &interval, ISC_TRUE);
-        }
+                       isc_interval_set(&interval, 0, 1);
+                       (void)isc_timer_reset(adb->timer, isc_timertype_once,
+                                             NULL, &interval, ISC_TRUE);
+               }
 #endif
+               isc_mem_waterack(adb->mctx, mark);
+       }
+       UNLOCK(&adb->overmemlock);
 }
 
 void

diff --git a/lib/dns/cache.c b/lib/dns/cache.c
index cd3842050936f9118ec7c9520fedd6295609157f..fb03429347dfd72766fe00b1ab677d267b2b6f8a 100644 (file)
--- a/lib/dns/cache.c
+++ b/lib/dns/cache.c
@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: cache.c,v 1.76 2007/10/19 17:15:53 explorer Exp $ */
+/* $Id: cache.c,v 1.77 2008/02/07 02:41:26 marka Exp $ */
 
 /*! \file */
 
@@ -708,8 +708,11 @@ water(void *arg, int mark) {
 
         LOCK(&cache->cleaner.lock);
 
-        dns_db_overmem(cache->db, overmem);
-        cache->cleaner.overmem = overmem;
+       if (overmem != cache->cleaner.overmem) {
+               dns_db_overmem(cache->db, overmem);
+               cache->cleaner.overmem = overmem;
+               isc_mem_waterack(cache->mctx, mark);
+       }
 
         UNLOCK(&cache->cleaner.lock);
 }

diff --git a/lib/isc/include/isc/mem.h b/lib/isc/include/isc/mem.h
index 4a401ceb458d3af5a8b98ec3ad3b1291412f7dad..af8141a7207bead10dae8219ced6c32937a4cbf2 100644 (file)
--- a/lib/isc/include/isc/mem.h
+++ b/lib/isc/include/isc/mem.h
@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: mem.h,v 1.75 2008/01/18 23:46:58 tbox Exp $ */
+/* $Id: mem.h,v 1.76 2008/02/07 02:41:26 marka Exp $ */
 
 #ifndef ISC_MEM_H
 #define ISC_MEM_H 1
@@ -339,10 +339,27 @@ isc_mem_setwater(isc_mem_t *mctx, isc_mem_water_t water, void *water_arg,
 /*%<
  * Set high and low water marks for this memory context.
  *
- * When the memory
- * usage of 'mctx' exceeds 'hiwater', '(water)(water_arg, #ISC_MEM_HIWATER)'
- * will be called.  When the usage drops below 'lowater', 'water' will
- * again be called, this time with #ISC_MEM_LOWATER.
+ * When the memory usage of 'mctx' exceeds 'hiwater',
+ * '(water)(water_arg, #ISC_MEM_HIWATER)' will be called.  'water' needs to
+ * call isc_mem_waterack() with #ISC_MEM_HIWATER to acknowlege the state
+ * change.  'water' may be called multiple times.
+ *
+ * When the usage drops below 'lowater', 'water' will again be called, this
+ * time with #ISC_MEM_LOWATER.  'water' need to calls isc_mem_waterack() with
+ * #ISC_MEM_LOWATER to acknowlege the change.
+ *
+ *     static void
+ *     water(void *arg, int mark) {
+ *             struct foo *foo = *arg;
+ *
+ *             LOCK(&foo->marklock);
+ *             if (foo->mark != mark) {
+ *                     foo->mark = mark;
+ *                     ....
+ *                     isc_mem_waterack(foo->mctx, mark);
+ *             }
+ *             UNLOCK(&foo->marklock);
+ *     }
  *
  * If 'water' is NULL then 'water_arg', 'hi_water' and 'lo_water' are
  * ignored and the state is reset.
@@ -353,6 +370,12 @@ isc_mem_setwater(isc_mem_t *mctx, isc_mem_water_t water, void *water_arg,
  *     hi_water >= lo_water
  */
 
+void
+isc_mem_waterack(isc_mem_t *ctx, int mark);
+/*%<
+ * Called to acknowledge changes in signalled by calls to 'water'.
+ */
+
 void
 isc_mem_printactive(isc_mem_t *mctx, FILE *file);
 /*%<

diff --git a/lib/isc/mem.c b/lib/isc/mem.c
index b949ce9020527d7816596d00ab23afe58d28f150..4b14ad0ef9e0722419c18881dc21ef2584341ffb 100644 (file)
--- a/lib/isc/mem.c
+++ b/lib/isc/mem.c
@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: mem.c,v 1.140 2008/01/18 23:46:58 tbox Exp $ */
+/* $Id: mem.c,v 1.141 2008/02/07 02:41:26 marka Exp $ */
 
 /*! \file */
 
@@ -1086,7 +1086,6 @@ isc__mem_get(isc_mem_t *ctx, size_t size FLARG) {
        ADD_TRACE(ctx, ptr, size, file, line);
        if (ctx->hi_water != 0U && !ctx->hi_called &&
            ctx->inuse > ctx->hi_water) {
-               ctx->hi_called = ISC_TRUE;
                call_water = ISC_TRUE;
        }
        if (ctx->inuse > ctx->maxinuse) {
@@ -1144,8 +1143,6 @@ isc__mem_put(isc_mem_t *ctx, void *ptr, size_t size FLARG)
         */
        if (ctx->hi_called &&
            (ctx->inuse < ctx->lo_water || ctx->lo_water == 0U)) {
-               ctx->hi_called = ISC_FALSE;
-
                if (ctx->water != NULL)
                        call_water = ISC_TRUE;
        }
@@ -1155,6 +1152,18 @@ isc__mem_put(isc_mem_t *ctx, void *ptr, size_t size FLARG)
                (ctx->water)(ctx->water_arg, ISC_MEM_LOWATER);
 }
 
+void
+isc_mem_waterack(isc_mem_t *ctx, int flag) {
+       REQUIRE(VALID_CONTEXT(ctx));
+
+       MCTXLOCK(ctx, &ctx->lock);
+       if (flag == ISC_MEM_LOWATER)
+               ctx->hi_called = ISC_FALSE;
+       else if (flag == ISC_MEM_HIWATER)
+               ctx->hi_called = ISC_TRUE;
+       MCTXUNLOCK(ctx, &ctx->lock);
+}
+
 #if ISC_MEM_TRACKLINES
 static void
 print_active(isc_mem_t *mctx, FILE *out) {

diff --git a/lib/isc/win32/libisc.def b/lib/isc/win32/libisc.def
index 42cb272b449956d8c94c493dce1c2b65897bcc00..1480d85e92dd2c67b4f9e78d3acaafe256f5b744 100644 (file)
--- a/lib/isc/win32/libisc.def
+++ b/lib/isc/win32/libisc.def
@@ -255,6 +255,7 @@ isc_mem_setdestroycheck
 isc_mem_setquota
 isc_mem_setwater
 isc_mem_stats
+isc_mem_waterack
 isc_mempool_associatelock
 isc_mempool_create
 isc_mempool_destroy