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.

diff --git a/lib/dns/adb.c b/lib/dns/adb.c
index 6b1bddad636c67cf4ed2622d23642756ab5e9f2e..e7fee8d38efba0406d641fc88fb258cfdaa818f6 100644 (file)
--- a/lib/dns/adb.c
+++ b/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);
        }

diff --git a/lib/dns/cache.c b/lib/dns/cache.c
index bf73300c0584db15bb6ed00b757b9b95481c6a0d..26af46d1a2a51b307349a69c7181a6b4145cfa29 100644 (file)
--- a/lib/dns/cache.c
+++ b/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

diff --git a/lib/isc/include/isc/mem.h b/lib/isc/include/isc/mem.h
index 4937de070af17dbf0bcb2ceea9b2d4e954a3b6bf..0e0d24a3b16f9ec5bdc8a2721d193ea5fb3e1cd0 100644 (file)
--- a/lib/isc/include/isc/mem.h
+++ b/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

diff --git a/lib/isc/mem.c b/lib/isc/mem.c
index 4e92e54c97d3e2094315803127ffe9fde49dea54..e598afce8b0c9303e38f6d1cb759390fc9a1d69b 100644 (file)
--- a/lib/isc/mem.c
+++ b/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,
-               };
+       /* No water was set and new water is also NULL */
+       if (oldwater == NULL && water == NULL) {
+               return;
        }
-       oldwater = (water_t *)atomic_exchange(&ctx->water, (uintptr_t)newwater);
 
+       /* The water function is being set for the first time */
        if (oldwater == NULL) {
+               REQUIRE(water != NULL && lowater > 0);
+
+               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;
        }
 
-       INSIST(oldwater->water != NULL);
+       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) &&
-           (oldwater->water != water || oldwater->water_arg != water_arg ||
-            atomic_load_acquire(&ctx->inuse) < lowater || lowater == 0U))
+           (atomic_load_acquire(&ctx->inuse) < lowater || lowater == 0U))
        {
-               (oldwater->water)(oldwater->water_arg, ISC_MEM_LOWATER);
+               (oldwater)(oldwater_arg, ISC_MEM_LOWATER);
        }
-
-       decrement_malloced(ctx, sizeof(*oldwater));
-       sdallocx(oldwater, sizeof(*oldwater), 0);
 }
 
 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);