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.
2025-09-02 23:55:27 +00:00 · 2008-02-07 02:41:26 +00:00
parent e934a311cb
commit 9a7d202077
7 changed files with 88 additions and 24 deletions
--- a/7
+++ b/7
@@ -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,
--- 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);

+	if (acache->cleaner.overmem != overmem) {
 		acache->cleaner.overmem = overmem;

 		if (acache->cleaner.overmem_event != NULL)
-		isc_task_send(acache->task, &acache->cleaner.overmem_event);
+			isc_task_send(acache->task,
+				      &acache->cleaner.overmem_event);
+		isc_mem_waterack(acache->mctx, mark);
+	}

 	UNLOCK(&acache->cleaner.lock);
 }
--- 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");

+	/*
+	 * 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;

 			isc_interval_set(&interval, 0, 1);
-                (void)isc_timer_reset(adb->timer, isc_timertype_once, NULL,
-                                      &interval, ISC_TRUE);
+			(void)isc_timer_reset(adb->timer, isc_timertype_once,
+					      NULL, &interval, ISC_TRUE);
 		}
 #endif
+		isc_mem_waterack(adb->mctx, mark);
+	}
+	UNLOCK(&adb->overmemlock);
 }

 void
--- 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);

+	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);
 }
--- 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);
 /*%<
--- 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) {
--- 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