Use static storage for isc_mem water_t

On the isc_mem water change the old water_t structure could be used
after free.  Instead of introducing reference counting on the hot-path
we are going to introduce additional constraints on the
isc_mem_setwater.  Once it's set for the first time, the additional
calls have to be made with the same water and water_arg arguments.

lib/dns/adb.c

@@ -2869,7 +2869,7 @@ dns_adb_shutdown(dns_adb_t *adb) {
 
 	if (!adb->shutting_down) {
 		adb->shutting_down = true;
-		isc_mem_setwater(adb->mctx, NULL, NULL, 0, 0);
+		isc_mem_clearwater(adb->mctx);
 		/*
 		 * Isolate shutdown_names and shutdown_entries calls.
 		 */
@@ -4699,7 +4699,7 @@ dns_adb_setadbsize(dns_adb_t *adb, size_t size) {
 	lowater = size - (size >> 2); /* Approximately 3/4ths. */
 
 	if (size == 0U || hiwater == 0U || lowater == 0U) {
-		isc_mem_setwater(adb->mctx, NULL, NULL, 0, 0);
+		isc_mem_clearwater(adb->mctx);
 	} else {
 		isc_mem_setwater(adb->mctx, water, adb, hiwater, lowater);
 	}

lib/dns/cache.c

@@ -167,6 +167,9 @@ cleaner_shutdown_action(isc_task_t *task, isc_event_t *event);
 static void
 overmem_cleaning_action(isc_task_t *task, isc_event_t *event);
 
+static void
+water(void *arg, int mark);
+
 static inline isc_result_t
 cache_create_db(dns_cache_t *cache, dns_db_t **db) {
 	isc_result_t result;
@@ -328,7 +331,7 @@ cache_free(dns_cache_t *cache) {
 	isc_refcount_destroy(&cache->references);
 	isc_refcount_destroy(&cache->live_tasks);
 
-	isc_mem_setwater(cache->mctx, NULL, NULL, 0, 0);
+	isc_mem_clearwater(cache->mctx);
 
 	if (cache->cleaner.task != NULL) {
 		isc_task_detach(&cache->cleaner.task);
@@ -950,7 +953,7 @@ dns_cache_setcachesize(dns_cache_t *cache, size_t size) {
 		/*
 		 * Disable cache memory limiting.
 		 */
-		isc_mem_setwater(cache->mctx, NULL, NULL, 0, 0);
+		isc_mem_clearwater(cache->mctx);
 	} else {
 		/*
 		 * Establish new cache memory limits (either for the first

lib/isc/include/isc/mem.h

@@ -256,6 +256,8 @@ isc_mem_isovermem(isc_mem_t *mctx);
  * the mark.
  */
 
+void
+isc_mem_clearwater(isc_mem_t *mctx);
 void
 isc_mem_setwater(isc_mem_t *mctx, isc_mem_water_t water, void *water_arg,
 		 size_t hiwater, size_t lowater);
@@ -264,12 +266,12 @@ isc_mem_setwater(isc_mem_t *mctx, isc_mem_water_t water, void *water_arg,
  *
  * 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 acknowledge the
- *state change.  'water' may be called multiple times.
+ * to call isc_mem_waterack() with #ISC_MEM_HIWATER to acknowledge 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 acknowledge the change.
+ * this time with #ISC_MEM_LOWATER.  'water' need to calls
+ * isc_mem_waterack() with #ISC_MEM_LOWATER to acknowledge the change.
  *
  *	static void
  *	water(void *arg, int mark) {
@@ -277,20 +279,23 @@ isc_mem_setwater(isc_mem_t *mctx, isc_mem_water_t water, void *water_arg,
  *
  *		LOCK(&foo->marklock);
  *		if (foo->mark != mark) {
- * 			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.
+ * if 'water' is set to NULL, the 'hiwater' and 'lowater' must set to 0, and
+ * high- and low-water processing are disabled for this memory context.  There's
+ * a convenient function isc_mem_clearwater().
  *
  * Requires:
  *
- *	'water' is not NULL.
- *	hi_water >= lo_water
+ *\li   If 'water' is NULL, 'hiwater' and 'lowater' must be set to 0.
+ *\li	If 'water' and 'water_arg' have previously been set, they are
+	unchanged.
+ *\li	'hiwater' >= 'lowater'
  */
 
 void

lib/isc/mem.c

@@ -108,13 +108,6 @@ struct stats {
 	atomic_size_t totalgets;
 };
 
-typedef struct water {
-	isc_mem_water_t water;
-	void *water_arg;
-	size_t hi_water;
-	size_t lo_water;
-} water_t;
-
 #define MEM_MAGIC	 ISC_MAGIC('M', 'e', 'm', 'C')
 #define VALID_CONTEXT(c) ISC_MAGIC_VALID(c, MEM_MAGIC)
 
@@ -147,7 +140,10 @@ struct isc_mem {
 	atomic_size_t maxmalloced;
 	atomic_bool hi_called;
 	atomic_bool is_overmem;
-	atomic_uintptr_t water;
+	isc_mem_water_t water;
+	void *water_arg;
+	atomic_size_t hi_water;
+	atomic_size_t lo_water;
 	ISC_LIST(isc_mempool_t) pools;
 	unsigned int poolcnt;
 
@@ -443,7 +439,8 @@ mem_create(isc_mem_t **ctxp, unsigned int flags) {
 	atomic_init(&ctx->maxinuse, 0);
 	atomic_init(&ctx->malloced, sizeof(*ctx));
 	atomic_init(&ctx->maxmalloced, sizeof(*ctx));
-	atomic_init(&ctx->water, (uintptr_t)NULL);
+	atomic_init(&ctx->hi_water, 0);
+	atomic_init(&ctx->lo_water, 0);
 	atomic_init(&ctx->hi_called, false);
 	atomic_init(&ctx->is_overmem, false);
 
@@ -482,19 +479,12 @@ static void
 destroy(isc_mem_t *ctx) {
 	unsigned int i;
 	size_t malloced;
-	water_t *water;
 
 	LOCK(&contextslock);
 	ISC_LIST_UNLINK(contexts, ctx, link);
 	totallost += isc_mem_inuse(ctx);
 	UNLOCK(&contextslock);
 
-	water = (water_t *)atomic_exchange(&ctx->water, (uintptr_t)NULL);
-	if (water != NULL) {
-		sdallocx(water, sizeof(*water), 0);
-		decrement_malloced(ctx, sizeof(water_t));
-	}
-
 	ctx->magic = 0;
 
 	INSIST(ISC_LIST_EMPTY(ctx->pools));
@@ -647,33 +637,32 @@ isc__mem_destroy(isc_mem_t **ctxp FLARG) {
 	*ctxp = NULL;
 }
 
-#define CALL_HI_WATER(ctx)                                                    \
-	{                                                                     \
-		water_t *water = (water_t *)atomic_load_relaxed(&ctx->water); \
-		if (water != NULL && hi_water(ctx, water)) {                  \
-			(water->water)(water->water_arg, ISC_MEM_HIWATER);    \
-		}                                                             \
+#define CALL_HI_WATER(ctx)                                             \
+	{                                                              \
+		if (ctx->water != NULL && hi_water(ctx)) {             \
+			(ctx->water)(ctx->water_arg, ISC_MEM_HIWATER); \
+		}                                                      \
 	}
 
-#define CALL_LO_WATER(ctx)                                                    \
-	{                                                                     \
-		water_t *water = (water_t *)atomic_load_relaxed(&ctx->water); \
-		if ((water != NULL) && lo_water(ctx, water)) {                \
-			(water->water)(water->water_arg, ISC_MEM_LOWATER);    \
-		}                                                             \
+#define CALL_LO_WATER(ctx)                                             \
+	{                                                              \
+		if ((ctx->water != NULL) && lo_water(ctx)) {           \
+			(ctx->water)(ctx->water_arg, ISC_MEM_LOWATER); \
+		}                                                      \
 	}
 
 static inline bool
-hi_water(isc_mem_t *ctx, water_t *water) {
+hi_water(isc_mem_t *ctx) {
 	size_t inuse;
 	size_t maxinuse;
+	size_t hiwater = atomic_load_relaxed(&ctx->hi_water);
 
-	if (water->hi_water == 0) {
+	if (hiwater == 0) {
 		return (false);
 	}
 
 	inuse = atomic_load_acquire(&ctx->inuse);
-	if (inuse <= water->hi_water) {
+	if (inuse <= hiwater) {
 		return (false);
 	}
 
@@ -699,15 +688,16 @@ hi_water(isc_mem_t *ctx, water_t *water) {
 }
 
 static inline bool
-lo_water(isc_mem_t *ctx, water_t *water) {
+lo_water(isc_mem_t *ctx) {
 	size_t inuse;
+	size_t lowater = atomic_load_relaxed(&ctx->lo_water);
 
-	if (water->lo_water == 0) {
+	if (lowater == 0) {
 		return (false);
 	}
 
 	inuse = atomic_load_acquire(&ctx->inuse);
-	if (inuse >= water->lo_water) {
+	if (inuse >= lowater) {
 		return (false);
 	}
 
@@ -1035,43 +1025,54 @@ isc_mem_maxmalloced(isc_mem_t *ctx) {
 	return (atomic_load_acquire(&ctx->maxmalloced));
 }
 
+void
+isc_mem_clearwater(isc_mem_t *mctx) {
+	isc_mem_setwater(mctx, NULL, NULL, 0, 0);
+}
+
 void
 isc_mem_setwater(isc_mem_t *ctx, isc_mem_water_t water, void *water_arg,
 		 size_t hiwater, size_t lowater) {
-	water_t *oldwater;
-	water_t *newwater = NULL;
+	isc_mem_water_t oldwater;
+	void *oldwater_arg;
 
 	REQUIRE(VALID_CONTEXT(ctx));
 	REQUIRE(hiwater >= lowater);
 
-	if (water != NULL) {
-		newwater = mallocx(sizeof(*newwater), 0);
-		increment_malloced(ctx, sizeof(*newwater));
+	oldwater = ctx->water;
+	oldwater_arg = ctx->water_arg;
 
-		*newwater = (water_t){
-			.water = water,
-			.water_arg = water_arg,
-			.hi_water = hiwater,
-			.lo_water = lowater,
-		};
-	}
-	oldwater = (water_t *)atomic_exchange(&ctx->water, (uintptr_t)newwater);
-
-	if (oldwater == NULL) {
+	/* No water was set and new water is also NULL */
+	if (oldwater == NULL && water == NULL) {
 		return;
 	}
 
-	INSIST(oldwater->water != NULL);
+	/* The water function is being set for the first time */
+	if (oldwater == NULL) {
+		REQUIRE(water != NULL && lowater > 0);
 
-	if (atomic_load_acquire(&ctx->hi_called) &&
-	    (oldwater->water != water || oldwater->water_arg != water_arg ||
-	     atomic_load_acquire(&ctx->inuse) < lowater || lowater == 0U))
-	{
-		(oldwater->water)(oldwater->water_arg, ISC_MEM_LOWATER);
+		INSIST(atomic_load(&ctx->hi_water) == 0);
+		INSIST(atomic_load(&ctx->lo_water) == 0);
+
+		ctx->water = water;
+		ctx->water_arg = water_arg;
+		atomic_store(&ctx->hi_water, hiwater);
+		atomic_store(&ctx->lo_water, lowater);
+
+		return;
 	}
 
-	decrement_malloced(ctx, sizeof(*oldwater));
-	sdallocx(oldwater, sizeof(*oldwater), 0);
+	REQUIRE((water == oldwater && water_arg == oldwater_arg) ||
+		(water == NULL && water_arg == NULL && hiwater == 0));
+
+	atomic_store(&ctx->hi_water, hiwater);
+	atomic_store(&ctx->lo_water, lowater);
+
+	if (atomic_load_acquire(&ctx->hi_called) &&
+	    (atomic_load_acquire(&ctx->inuse) < lowater || lowater == 0U))
+	{
+		(oldwater)(oldwater_arg, ISC_MEM_LOWATER);
+	}
 }
 
 bool
@@ -1431,7 +1432,6 @@ xml_renderctx(isc_mem_t *ctx, summarystat_t *summary, xmlTextWriterPtr writer) {
 	REQUIRE(VALID_CONTEXT(ctx));
 
 	int xmlrc;
-	water_t *water;
 
 	MCTXLOCK(ctx);
 
@@ -1495,16 +1495,15 @@ xml_renderctx(isc_mem_t *ctx, summarystat_t *summary, xmlTextWriterPtr writer) {
 	summary->contextsize += ctx->poolcnt * sizeof(isc_mempool_t);
 
 	TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "hiwater"));
-	water = (water_t *)atomic_load_relaxed(&ctx->water);
 	TRY0(xmlTextWriterWriteFormatString(
 		writer, "%" PRIu64 "",
-		(water != NULL) ? (uint64_t)water->hi_water : 0));
+		(uint64_t)atomic_load_relaxed(&ctx->hi_water)));
 	TRY0(xmlTextWriterEndElement(writer)); /* hiwater */
 
 	TRY0(xmlTextWriterStartElement(writer, ISC_XMLCHAR "lowater"));
 	TRY0(xmlTextWriterWriteFormatString(
 		writer, "%" PRIu64 "",
-		(water != NULL) ? (uint64_t)water->lo_water : 0));
+		(uint64_t)atomic_load_relaxed(&ctx->lo_water)));
 	TRY0(xmlTextWriterEndElement(writer)); /* lowater */
 
 	TRY0(xmlTextWriterEndElement(writer)); /* context */
@@ -1583,7 +1582,6 @@ json_renderctx(isc_mem_t *ctx, summarystat_t *summary, json_object *array) {
 
 	json_object *ctxobj, *obj;
 	char buf[1024];
-	water_t *water;
 
 	MCTXLOCK(ctx);
 
@@ -1643,12 +1641,11 @@ json_renderctx(isc_mem_t *ctx, summarystat_t *summary, json_object *array) {
 
 	summary->contextsize += ctx->poolcnt * sizeof(isc_mempool_t);
 
-	water = (water_t *)atomic_load_relaxed(&ctx->water);
-	obj = json_object_new_int64((water != NULL) ? water->hi_water : 0);
+	obj = json_object_new_int64(atomic_load_relaxed(&ctx->hi_water));
 	CHECKMEM(obj);
 	json_object_object_add(ctxobj, "hiwater", obj);
 
-	obj = json_object_new_int64((water != NULL) ? water->lo_water : 0);
+	obj = json_object_new_int64(atomic_load_relaxed(&ctx->lo_water));
 	CHECKMEM(obj);
 	json_object_object_add(ctxobj, "lowater", obj);