fix a "max-cache-size" configuration bug

"max-cache-size default;" is allowed, according to the documentation
and the parser, but when it's configured, named crashes due to an
INSIST that the only legal string value is "unlimited". this has
been fied.

the configuration has also been simplified. previously, we checked for
max-cache-size in view and options, then determined whether to look in
the global default options based on whether the view had recursion set.
the default value set there was only applicable to views with recursion.
now, the default is an explicit "default", which affects views with
and without recursion in different ways.

the cfg type for "max-cache-size" has been changed from
cfg_type_sizeorpercent to cfg_type_maxcachesize.

diff --git a/bin/named/config.c b/bin/named/config.c
index 00006a8522f97a70d0e532719490b316fab20490..9870698402255f77789900d729bfb3538c9656bf 100644 (file)
--- a/bin/named/config.c
+++ b/bin/named/config.c
@@ -168,7 +168,7 @@ options {\n\
 #ifdef HAVE_LMDB
                            "   lmdb-mapsize 32M;\n"
 #endif /* ifdef HAVE_LMDB */
-                           "   max-cache-size 90%;\n\
+                           "   max-cache-size default;\n\
        max-cache-ttl 604800; /* 1 week */\n\
        max-clients-per-query 100;\n\
        max-ncache-ttl 10800; /* 3 hours */\n\

diff --git a/bin/named/server.c b/bin/named/server.c
index 5d7f74cb1933cb6aaf2b2f12e0b6fe577a7a6850..c3dca714b211594f90dfe5ad31ca3c6bbb0361b4 100644 (file)
--- a/bin/named/server.c
+++ b/bin/named/server.c
@@ -3742,42 +3742,6 @@ named_register_one_plugin(const cfg_obj_t *config, const cfg_obj_t *obj,
        return result;
 }
 
-/*
- * Determine if a minimal-sized cache can be used for a given view, according
- * to 'maps' (implicit defaults, global options, view options) and 'optionmaps'
- * (global options, view options).  This is only allowed for views which have
- * recursion disabled and do not have "max-cache-size" set explicitly.  Using
- * minimal-sized caches prevents a situation in which all explicitly configured
- * and built-in views inherit the default "max-cache-size 90%;" setting, which
- * could lead to memory exhaustion with multiple views configured.
- */
-static bool
-minimal_cache_allowed(const cfg_obj_t *maps[4],
-                     const cfg_obj_t *optionmaps[3]) {
-       const cfg_obj_t *obj;
-
-       /*
-        * Do not use a minimal-sized cache for a view with recursion enabled.
-        */
-       obj = NULL;
-       (void)named_config_get(maps, "recursion", &obj);
-       INSIST(obj != NULL);
-       if (cfg_obj_asboolean(obj)) {
-               return false;
-       }
-
-       /*
-        * Do not use a minimal-sized cache if a specific size was requested.
-        */
-       obj = NULL;
-       (void)named_config_get(optionmaps, "max-cache-size", &obj);
-       if (obj != NULL) {
-               return false;
-       }
-
-       return true;
-}
-
 static const char *const response_synonyms[] = { "response", NULL };
 
 /*
@@ -4040,42 +4004,60 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
         * we can reuse/share an existing cache.
         */
        obj = NULL;
-       result = named_config_get(maps, "max-cache-size", &obj);
+       result = named_config_get(maps, "recursion", &obj);
        INSIST(result == ISC_R_SUCCESS);
-       /*
-        * If "-T maxcachesize=..." is in effect, it overrides any other
-        * "max-cache-size" setting found in configuration, either implicit or
-        * explicit.  For simplicity, the value passed to that command line
-        * option is always treated as the number of bytes to set
-        * "max-cache-size" to.
-        */
+       view->recursion = cfg_obj_asboolean(obj);
+
        if (named_g_maxcachesize != 0) {
-               max_cache_size = named_g_maxcachesize;
-       } else if (minimal_cache_allowed(maps, optionmaps)) {
                /*
-                * dns_cache_setcachesize() will adjust this to the smallest
-                * allowed value.
+                * If "-T maxcachesize=..." is in effect, it overrides any
+                * other "max-cache-size" setting found in configuration,
+                * either implicit or explicit.  For simplicity, the value
+                * passed to that command line option is always treated as
+                * the number of bytes to set "max-cache-size" to.
                 */
-               max_cache_size = 1;
-       } else if (cfg_obj_isstring(obj)) {
-               str = cfg_obj_asstring(obj);
-               INSIST(strcasecmp(str, "unlimited") == 0);
-               max_cache_size = 0;
-       } else if (cfg_obj_ispercentage(obj)) {
-               max_cache_size = SIZE_AS_PERCENT;
-               max_cache_size_percent = cfg_obj_aspercentage(obj);
-       } else {
-               uint64_t value = cfg_obj_asuint64(obj);
-               if (value > SIZE_MAX) {
-                       cfg_obj_log(obj, ISC_LOG_WARNING,
-                                   "'max-cache-size "
-                                   "%" PRIu64 "' "
-                                   "is too large for this "
-                                   "system; reducing to %lu",
-                                   value, (unsigned long)SIZE_MAX);
-                       value = SIZE_MAX;
+               max_cache_size = named_g_maxcachesize;
+       } else {
+               obj = NULL;
+               result = named_config_get(maps, "max-cache-size", &obj);
+               INSIST(result == ISC_R_SUCCESS);
+               if (cfg_obj_isstring(obj) &&
+                   strcasecmp(cfg_obj_asstring(obj), "default") == 0)
+               {
+                       /*
+                        * The default for a view with recursion
+                        * is 90% of memory. With no recursion,
+                        * it's the minimum cache size allowed by
+                        * dns_cache_setcachesize().
+                        */
+                       if (view->recursion) {
+                               max_cache_size = SIZE_AS_PERCENT;
+                               max_cache_size_percent = 90;
+                       } else {
+                               max_cache_size = 1;
+                       }
+               } else if (cfg_obj_isstring(obj)) {
+                       str = cfg_obj_asstring(obj);
+                       INSIST(strcasecmp(str, "unlimited") == 0);
+                       max_cache_size = 0;
+               } else if (cfg_obj_ispercentage(obj)) {
+                       max_cache_size = SIZE_AS_PERCENT;
+                       max_cache_size_percent = cfg_obj_aspercentage(obj);
+               } else if (cfg_obj_isuint64(obj)) {
+                       uint64_t value = cfg_obj_asuint64(obj);
+                       if (value > SIZE_MAX) {
+                               cfg_obj_log(obj, ISC_LOG_WARNING,
+                                           "'max-cache-size "
+                                           "%" PRIu64 "' "
+                                           "is too large for this "
+                                           "system; reducing to %lu",
+                                           value, (unsigned long)SIZE_MAX);
+                               value = SIZE_MAX;
+                       }
+                       max_cache_size = (size_t)value;
+               } else {
+                       UNREACHABLE();
                }
-               max_cache_size = (size_t)value;
        }
 
        if (max_cache_size == SIZE_AS_PERCENT) {
@@ -4879,11 +4861,6 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config,
        /*
         * Configure other configurable data.
         */
-       obj = NULL;
-       result = named_config_get(maps, "recursion", &obj);
-       INSIST(result == ISC_R_SUCCESS);
-       view->recursion = cfg_obj_asboolean(obj);
-
        obj = NULL;
        result = named_config_get(maps, "qname-minimization", &obj);
        INSIST(result == ISC_R_SUCCESS);

diff --git a/lib/isccfg/namedconf.c b/lib/isccfg/namedconf.c
index 5d0117ac5aced940571d8257694178067667af17..4b7a203ddb4a619eb9f5171bf92edfa16ceb835b 100644 (file)
--- a/lib/isccfg/namedconf.c
+++ b/lib/isccfg/namedconf.c
@@ -111,6 +111,7 @@ static cfg_type_t cfg_type_logseverity;
 static cfg_type_t cfg_type_logsuffix;
 static cfg_type_t cfg_type_logversions;
 static cfg_type_t cfg_type_remoteselement;
+static cfg_type_t cfg_type_maxcachesize;
 static cfg_type_t cfg_type_maxduration;
 static cfg_type_t cfg_type_minimal;
 static cfg_type_t cfg_type_nameportiplist;
@@ -138,7 +139,6 @@ static cfg_type_t cfg_type_server;
 static cfg_type_t cfg_type_server_key_kludge;
 static cfg_type_t cfg_type_size;
 static cfg_type_t cfg_type_sizenodefault;
-static cfg_type_t cfg_type_sizeorpercent;
 static cfg_type_t cfg_type_sizeval;
 static cfg_type_t cfg_type_sockaddr4wild;
 static cfg_type_t cfg_type_sockaddr6wild;
@@ -2091,7 +2091,7 @@ static cfg_clausedef_t view_clauses[] = {
        { "lmdb-mapsize", &cfg_type_sizeval, CFG_CLAUSEFLAG_NOTCONFIGURED },
 #endif /* ifdef HAVE_LMDB */
        { "max-acache-size", NULL, CFG_CLAUSEFLAG_ANCIENT },
-       { "max-cache-size", &cfg_type_sizeorpercent, 0 },
+       { "max-cache-size", &cfg_type_maxcachesize, 0 },
        { "max-cache-ttl", &cfg_type_duration, 0 },
        { "max-clients-per-query", &cfg_type_uint32, 0 },
        { "max-ncache-ttl", &cfg_type_duration, 0 },
@@ -2932,14 +2932,14 @@ static cfg_type_t cfg_type_sizeval_percent = {
  */
 
 static isc_result_t
-parse_size_or_percent(cfg_parser_t *pctx, const cfg_type_t *type,
-                     cfg_obj_t **ret) {
+parse_maxcachesize(cfg_parser_t *pctx, const cfg_type_t *type,
+                  cfg_obj_t **ret) {
        return cfg_parse_enum_or_other(pctx, type, &cfg_type_sizeval_percent,
                                       ret);
 }
 
 static void
-doc_parse_size_or_percent(cfg_printer_t *pctx, const cfg_type_t *type) {
+doc_maxcachesize(cfg_printer_t *pctx, const cfg_type_t *type) {
        UNUSED(type);
        cfg_print_cstr(pctx, "( default | unlimited | ");
        cfg_doc_terminal(pctx, &cfg_type_sizeval);
@@ -2948,10 +2948,10 @@ doc_parse_size_or_percent(cfg_printer_t *pctx, const cfg_type_t *type) {
        cfg_print_cstr(pctx, " )");
 }
 
-static const char *sizeorpercent_enums[] = { "default", "unlimited", NULL };
-static cfg_type_t cfg_type_sizeorpercent = {
-       "size_or_percent",         parse_size_or_percent, cfg_print_ustring,
-       doc_parse_size_or_percent, &cfg_rep_string,       sizeorpercent_enums
+static const char *maxcachesize_enums[] = { "default", "unlimited", NULL };
+static cfg_type_t cfg_type_maxcachesize = {
+       "maxcachesize",   parse_maxcachesize, cfg_print_ustring,
+       doc_maxcachesize, &cfg_rep_string,    maxcachesize_enums
 };
 
 /*%