From: alessio Date: Fri, 29 Nov 2024 09:02:13 +0000 (+0100) Subject: Refactor and simplify isc_symtab X-Git-Tag: ondrej/lock-free-qpzone-reads-v1~59^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=53991ecc144d1a53181c4dee53643b6eb9a4dfe0;p=thirdparty%2Fbind9.git Refactor and simplify isc_symtab This commit does several changes to isc_symtab: 1. Rewrite the isc_symtab to internally use isc_hashmap instead of hand-stiched hashtable. 2. Create a new isc_symtab_define_and_return() api, which returns the already defined symvalue on ISC_R_EXISTS; this allows users of the API to skip the isc_symtab_lookup()+isc_symtab_define() calls and directly call isc_symtab_define_and_return(). 3. Merge isccc_symtab into isc_symtab - the only missing function was isccc_symtab_foreach() that was merged into isc_symtab API. 4. Add full set of unit tests for the isc_symtab API. --- diff --git a/bin/check/check-tool.c b/bin/check/check-tool.c index f502840f3fc..794160e04b5 100644 --- a/bin/check/check-tool.c +++ b/bin/check/check-tool.c @@ -112,8 +112,7 @@ add(char *key, int value) { } if (symtab == NULL) { - isc_symtab_create(sym_mctx, 100, freekey, sym_mctx, false, - &symtab); + isc_symtab_create(sym_mctx, freekey, sym_mctx, false, &symtab); } key = isc_mem_strdup(sym_mctx, key); diff --git a/bin/named/controlconf.c b/bin/named/controlconf.c index bdf347157ef..5f792d76c20 100644 --- a/bin/named/controlconf.c +++ b/bin/named/controlconf.c @@ -31,13 +31,13 @@ #include #include #include +#include #include #include #include #include #include -#include #include #include @@ -108,7 +108,7 @@ struct named_controls { controllistenerlist_t listeners; bool shuttingdown; isc_mutex_t symtab_lock; - isccc_symtab_t *symtab; + isc_symtab_t *symtab; }; static isc_result_t @@ -1336,7 +1336,6 @@ named_controls_configure(named_controls_t *cp, const cfg_obj_t *config, isc_result_t named_controls_create(named_server_t *server, named_controls_t **ctrlsp) { isc_mem_t *mctx = server->mctx; - isc_result_t result; named_controls_t *controls = isc_mem_get(mctx, sizeof(*controls)); *controls = (named_controls_t){ @@ -1347,14 +1346,9 @@ named_controls_create(named_server_t *server, named_controls_t **ctrlsp) { isc_mutex_init(&controls->symtab_lock); LOCK(&controls->symtab_lock); - result = isccc_cc_createsymtab(&controls->symtab); + isccc_cc_createsymtab(mctx, &controls->symtab); UNLOCK(&controls->symtab_lock); - if (result != ISC_R_SUCCESS) { - isc_mutex_destroy(&controls->symtab_lock); - isc_mem_put(server->mctx, controls, sizeof(*controls)); - return result; - } *ctrlsp = controls; return ISC_R_SUCCESS; } @@ -1368,7 +1362,7 @@ named_controls_destroy(named_controls_t **ctrlsp) { REQUIRE(ISC_LIST_EMPTY(controls->listeners)); LOCK(&controls->symtab_lock); - isccc_symtab_destroy(&controls->symtab); + isc_symtab_destroy(&controls->symtab); UNLOCK(&controls->symtab_lock); isc_mutex_destroy(&controls->symtab_lock); isc_mem_put(controls->server->mctx, controls, sizeof(*controls)); diff --git a/lib/isc/include/isc/symtab.h b/lib/isc/include/isc/symtab.h index 9dda1c10471..514cec3f66b 100644 --- a/lib/isc/include/isc/symtab.h +++ b/lib/isc/include/isc/symtab.h @@ -20,10 +20,9 @@ /*! \file isc/symtab.h * \brief Provides a simple memory-based symbol table. * - * Keys are C strings, and key comparisons are case-insensitive. A type may - * be specified when looking up, defining, or undefining. A type value of - * 0 means "match any type"; any other value will only match the given - * type. + * Keys are C strings, and key comparisons are either case-insensitive or + * case-sensitive (decided when the symtab is created). A type must be + * specified when looking up, defining, or undefining. * * It's possible that a client will attempt to define a * tuple when a tuple with the given key and type already exists in the table. @@ -33,51 +32,25 @@ *\li #isc_symexists_replace Replace the old value with the new. The * undefine action (if provided) will be called * with the old tuple. - *\li #isc_symexists_add Add the new tuple, leaving the old tuple in - * the table. Subsequent lookups will retrieve - * the most-recently-defined tuple. * - * A lookup of a key using type 0 will return the most-recently defined - * symbol with that key. An undefine of a key using type 0 will undefine the - * most-recently defined symbol with that key. Trying to define a key with - * type 0 is illegal. - * - * The symbol table library does not make a copy the key field, so the - * caller must ensure that any key it passes to isc_symtab_define() will not - * change until it calls isc_symtab_undefine() or isc_symtab_destroy(). + * The symbol table library does not make a copy the key field, so the caller + * must ensure that any key it passes to isc_symtab_define() will not change + * or become undefined until it calls isc_symtab_undefine() + * or isc_symtab_destroy(). * * A user-specified action will be called (if provided) when a symbol is * undefined. It can be used to free memory associated with keys and/or * values. * - * A symbol table is implemented as a hash table of lists; the size of the - * hash table is set by the 'size' parameter to isc_symtbl_create(). When - * the number of entries in the symbol table reaches three quarters of this - * value, the hash table is reallocated with size doubled, in order to - * optimize lookup performance. This has a negative effect on insertion - * performance, which can be mitigated by sizing the table appropriately - * when creating it. - * - * \li MP: - * The callers of this module must ensure any required synchronization. - * - * \li Reliability: - * No anticipated impact. - * - * \li Resources: - * TBS - * - * \li Security: - * No anticipated impact. - * - * \li Standards: - * None. + * A symbol table is implemented as a isc_hashmap; the bits of the + * hashmap is set by the 'size' parameter to isc_symtbl_create(). */ /*** *** Imports. ***/ +#include #include #include @@ -87,45 +60,120 @@ ***/ /*% Symbol table value. */ typedef union isc_symvalue { - void *as_pointer; - const void *as_cpointer; - int as_integer; - unsigned int as_uinteger; + void *as_pointer; + const void *as_cpointer; + intmax_t as_integer; + uintmax_t as_uinteger; } isc_symvalue_t; typedef void (*isc_symtabaction_t)(char *key, unsigned int type, isc_symvalue_t value, void *userarg); + +typedef bool (*isc_symtabforeachaction_t)(char *key, unsigned int type, + isc_symvalue_t value, void *userarg); + /*% Symbol table exists. */ typedef enum { isc_symexists_reject = 0, /*%< Disallow the define */ isc_symexists_replace = 1, /*%< Replace the old value with the new */ - isc_symexists_add = 2 /*%< Add the new tuple */ } isc_symexists_t; -/*% Create a symbol table. */ void -isc_symtab_create(isc_mem_t *mctx, unsigned int size, - isc_symtabaction_t undefine_action, void *undefine_arg, - bool case_sensitive, isc_symtab_t **symtabp); +isc_symtab_create(isc_mem_t *mctx, isc_symtabaction_t undefine_action, + void *undefine_arg, bool case_sensitive, + isc_symtab_t **symtabp); +/*!< + * \brief Create a symbol table. + * + * Requires: + * \li 'mctx' is valid memory context + * \li 'symtabp' is not NULL, `*symtabp' is NULL + */ -/*% Destroy a symbol table. */ void isc_symtab_destroy(isc_symtab_t **symtabp); +/*!< + * \brief Destroy a symbol table. + * + * Requires: + * \li '*symtabp' is a valid symbol table + */ -/*% Lookup a symbol table. */ isc_result_t isc_symtab_lookup(isc_symtab_t *symtab, const char *key, unsigned int type, - isc_symvalue_t *value); + isc_symvalue_t *found); +/*!< + * \brief Lookup a symbol table. + * + * Requires: + * \li 'symtab' is a valid symbol table + * \li 'key' is a valid C-string + * \li 'type' is not 0 + * \li 'found' is either NULL or a pointer to isc_symvalue_t + * + * Returns: + * \li #ISC_R_SUCCESS Symbol has been deleted from the symbol table + * \li #ISC_R_NOTFOUND Symbol not found in the symbol table + * + * Note: + * \li On success, if '*found' is not-NULL, it will be filled with value found + */ -/*% Define a symbol table. */ isc_result_t isc_symtab_define(isc_symtab_t *symtab, const char *key, unsigned int type, isc_symvalue_t value, isc_symexists_t exists_policy); -/*% Undefine a symbol table. */ +isc_result_t +isc_symtab_define_and_return(isc_symtab_t *symtab, const char *key, + unsigned int type, isc_symvalue_t value, + isc_symexists_t exists_policy, + isc_symvalue_t *found); +/*!< + * \brief Define a symbol table. + * + * Requires: + * \li 'symtab' is a valid symbol table + * \li 'key' is a valid C-string + * \li 'type' is not 0 + * \li 'exists_policy' is valid isc_symexist_t value + * \li 'found' is either NULL or a pointer to isc_symvalue_t + * + * Returns: + * \li #ISC_R_SUCCESS Symbol added to the symbol table + * \li #ISC_R_EXISTS Symbol already defined in the symbol table + * + * Note: + * \li On success, if '*found' is not-NULL, it will be filled with value added + * \li On exists, if '*found' is not-NULL, it will be fileed with value found + */ + isc_result_t isc_symtab_undefine(isc_symtab_t *symtab, const char *key, unsigned int type); +/*!< + * \brief Undefine a symbol table. + * + * Requires: + * \li 'symtab' is a valid symbol table + * \li 'key' is a valid C-string + * \li 'type' is not 0 + * + * Returns: + * \li #ISC_R_SUCCESS Symbol has been deleted from the symbol table + * \li #ISC_R_NOTFOUND Symbol not found in the symbol table + */ -/*% Return the number of items in a symbol table. */ unsigned int isc_symtab_count(isc_symtab_t *symtab); +/*!< + * \brief Return the number of items in a symbol table. + * + * Requires: + * \li 'symtab' is a valid symbol table + * + * Returns: + * \li number of items in a symbol table + */ + +void +isc_symtab_foreach(isc_symtab_t *symtab, isc_symtabforeachaction_t action, + void *arg); diff --git a/lib/isc/symtab.c b/lib/isc/symtab.c index bdd6a9d13c8..d4a7c5c6288 100644 --- a/lib/isc/symtab.c +++ b/lib/isc/symtab.c @@ -16,6 +16,8 @@ #include #include +#include +#include #include #include #include @@ -23,233 +25,205 @@ #include typedef struct elt { - char *key; + void *key; + size_t size; unsigned int type; isc_symvalue_t value; - LINK(struct elt) link; } elt_t; -typedef LIST(elt_t) eltlist_t; - -#define SYMTAB_MAGIC ISC_MAGIC('S', 'y', 'm', 'T') -#define VALID_SYMTAB(st) ISC_MAGIC_VALID(st, SYMTAB_MAGIC) +/* 7 bits means 128 entries at creation, which matches the common use of + * symtab */ +#define ISC_SYMTAB_INIT_HASH_BITS 7 +#define SYMTAB_MAGIC ISC_MAGIC('S', 'y', 'm', 'T') +#define VALID_SYMTAB(st) ISC_MAGIC_VALID(st, SYMTAB_MAGIC) struct isc_symtab { /* Unlocked. */ unsigned int magic; isc_mem_t *mctx; - unsigned int size; - unsigned int count; - unsigned int maxload; - eltlist_t *table; isc_symtabaction_t undefine_action; void *undefine_arg; + + isc_hashmap_t *hashmap; bool case_sensitive; }; -void -isc_symtab_create(isc_mem_t *mctx, unsigned int size, - isc_symtabaction_t undefine_action, void *undefine_arg, - bool case_sensitive, isc_symtab_t **symtabp) { - isc_symtab_t *symtab; - unsigned int i; +static void +elt_destroy(isc_symtab_t *symtab, elt_t *elt) { + if (symtab->undefine_action != NULL) { + (symtab->undefine_action)(elt->key, elt->type, elt->value, + symtab->undefine_arg); + } + isc_mem_put(symtab->mctx, elt, sizeof(*elt)); +} +void +isc_symtab_create(isc_mem_t *mctx, isc_symtabaction_t undefine_action, + void *undefine_arg, bool case_sensitive, + isc_symtab_t **symtabp) { REQUIRE(mctx != NULL); REQUIRE(symtabp != NULL && *symtabp == NULL); - REQUIRE(size > 0); /* Should be prime. */ - symtab = isc_mem_get(mctx, sizeof(*symtab)); + isc_symtab_t *symtab = isc_mem_get(mctx, sizeof(*symtab)); + *symtab = (isc_symtab_t){ + .undefine_action = undefine_action, + .undefine_arg = undefine_arg, + .case_sensitive = case_sensitive, + .magic = SYMTAB_MAGIC, + }; - symtab->mctx = NULL; isc_mem_attach(mctx, &symtab->mctx); - symtab->table = isc_mem_cget(mctx, size, sizeof(eltlist_t)); - for (i = 0; i < size; i++) { - INIT_LIST(symtab->table[i]); - } - symtab->size = size; - symtab->count = 0; - symtab->maxload = size * 3 / 4; - symtab->undefine_action = undefine_action; - symtab->undefine_arg = undefine_arg; - symtab->case_sensitive = case_sensitive; - symtab->magic = SYMTAB_MAGIC; + isc_hashmap_create(symtab->mctx, ISC_SYMTAB_INIT_HASH_BITS, + &symtab->hashmap); *symtabp = symtab; } void isc_symtab_destroy(isc_symtab_t **symtabp) { - isc_symtab_t *symtab; - unsigned int i; - elt_t *elt, *nelt; + REQUIRE(symtabp != NULL && VALID_SYMTAB(*symtabp)); - REQUIRE(symtabp != NULL); - symtab = *symtabp; + isc_result_t result; + isc_hashmap_iter_t *it = NULL; + isc_symtab_t *symtab = *symtabp; *symtabp = NULL; - REQUIRE(VALID_SYMTAB(symtab)); - for (i = 0; i < symtab->size; i++) { - for (elt = HEAD(symtab->table[i]); elt != NULL; elt = nelt) { - nelt = NEXT(elt, link); - if (symtab->undefine_action != NULL) { - (symtab->undefine_action)(elt->key, elt->type, - elt->value, - symtab->undefine_arg); - } - isc_mem_put(symtab->mctx, elt, sizeof(*elt)); - } - } - isc_mem_cput(symtab->mctx, symtab->table, symtab->size, - sizeof(eltlist_t)); symtab->magic = 0; + + isc_hashmap_iter_create(symtab->hashmap, &it); + for (result = isc_hashmap_iter_first(it); result == ISC_R_SUCCESS; + result = isc_hashmap_iter_delcurrent_next(it)) + { + elt_t *elt = NULL; + isc_hashmap_iter_current(it, (void **)&elt); + elt_destroy(symtab, elt); + } + INSIST(result == ISC_R_NOMORE); + isc_hashmap_iter_destroy(&it); + + isc_hashmap_destroy(&symtab->hashmap); + isc_mem_putanddetach(&symtab->mctx, symtab, sizeof(*symtab)); } -static unsigned int -hash(const char *key, bool case_sensitive) { - const char *s; - unsigned int h = 0; +static bool +elt__match(void *node, const void *key0, bool case_sensitive) { + const elt_t *elt = node; + const elt_t *key = key0; - /* - * This hash function is similar to the one Ousterhout - * uses in Tcl. - */ + if (elt->size != key->size) { + return false; + } + + if (elt->type != key->type) { + return false; + } if (case_sensitive) { - for (s = key; *s != '\0'; s++) { - h += (h << 3) + *s; - } + return memcmp(elt->key, key->key, key->size) == 0; } else { - for (s = key; *s != '\0'; s++) { - h += (h << 3) + isc_ascii_tolower(*s); - } + return isc_ascii_lowercmp(elt->key, key->key, key->size) == 0; } +} - return h; +static bool +elt_match_case(void *node, const void *key) { + return elt__match(node, key, true); } -#define FIND(s, k, t, b, e) \ - b = hash((k), (s)->case_sensitive) % (s)->size; \ - if ((s)->case_sensitive) { \ - for (e = HEAD((s)->table[b]); e != NULL; e = NEXT(e, link)) { \ - if (((t) == 0 || e->type == (t)) && \ - strcmp(e->key, (k)) == 0) \ - break; \ - } \ - } else { \ - for (e = HEAD((s)->table[b]); e != NULL; e = NEXT(e, link)) { \ - if (((t) == 0 || e->type == (t)) && \ - strcasecmp(e->key, (k)) == 0) \ - break; \ - } \ - } +static bool +elt_match_nocase(void *node, const void *key) { + return elt__match(node, key, false); +} + +static uint32_t +elt_hash(elt_t *elt, bool case_sensitive) { + isc_hash32_t hash; + + isc_hash32_init(&hash); + isc_hash32_hash(&hash, elt->key, elt->size, case_sensitive); + isc_hash32_hash(&hash, &elt->type, sizeof(elt->type), false); + return isc_hash32_finalize(&hash); +} isc_result_t isc_symtab_lookup(isc_symtab_t *symtab, const char *key, unsigned int type, - isc_symvalue_t *value) { - unsigned int bucket; - elt_t *elt; - + isc_symvalue_t *valuep) { REQUIRE(VALID_SYMTAB(symtab)); REQUIRE(key != NULL); + REQUIRE(type != 0); - FIND(symtab, key, type, bucket, elt); - - if (elt == NULL) { - return ISC_R_NOTFOUND; - } - - SET_IF_NOT_NULL(value, elt->value); - - return ISC_R_SUCCESS; -} - -static void -grow_table(isc_symtab_t *symtab) { - eltlist_t *newtable; - unsigned int i, newsize, newmax; - - REQUIRE(symtab != NULL); - - newsize = symtab->size * 2; - newmax = newsize * 3 / 4; - INSIST(newsize > 0U && newmax > 0U); - - newtable = isc_mem_cget(symtab->mctx, newsize, sizeof(eltlist_t)); - - for (i = 0; i < newsize; i++) { - INIT_LIST(newtable[i]); - } - - for (i = 0; i < symtab->size; i++) { - elt_t *elt, *nelt; - - for (elt = HEAD(symtab->table[i]); elt != NULL; elt = nelt) { - unsigned int hv; - - nelt = NEXT(elt, link); - - UNLINK(symtab->table[i], elt, link); - hv = hash(elt->key, symtab->case_sensitive); - APPEND(newtable[hv % newsize], elt, link); - } + elt_t *found = NULL; + elt_t elt = { + .key = UNCONST(key), + .size = strlen(key), + .type = type, + }; + uint32_t elt_hashval = elt_hash(&elt, symtab->case_sensitive); + isc_hashmap_match_fn elt_match = symtab->case_sensitive + ? elt_match_case + : elt_match_nocase; + isc_result_t result = isc_hashmap_find( + symtab->hashmap, elt_hashval, elt_match, &elt, (void **)&found); + + if (result == ISC_R_SUCCESS) { + SET_IF_NOT_NULL(valuep, found->value); } - isc_mem_cput(symtab->mctx, symtab->table, symtab->size, - sizeof(eltlist_t)); - - symtab->table = newtable; - symtab->size = newsize; - symtab->maxload = newmax; + return result; } isc_result_t isc_symtab_define(isc_symtab_t *symtab, const char *key, unsigned int type, isc_symvalue_t value, isc_symexists_t exists_policy) { - unsigned int bucket; - elt_t *elt; + return isc_symtab_define_and_return(symtab, key, type, value, + exists_policy, NULL); +} +isc_result_t +isc_symtab_define_and_return(isc_symtab_t *symtab, const char *key, + unsigned int type, isc_symvalue_t value, + isc_symexists_t exists_policy, + isc_symvalue_t *valuep) { REQUIRE(VALID_SYMTAB(symtab)); REQUIRE(key != NULL); REQUIRE(type != 0); - FIND(symtab, key, type, bucket, elt); - - if (exists_policy != isc_symexists_add && elt != NULL) { - if (exists_policy == isc_symexists_reject) { - return ISC_R_EXISTS; - } - INSIST(exists_policy == isc_symexists_replace); - UNLINK(symtab->table[bucket], elt, link); - if (symtab->undefine_action != NULL) { - (symtab->undefine_action)(elt->key, elt->type, - elt->value, - symtab->undefine_arg); - } - } else { - elt = isc_mem_get(symtab->mctx, sizeof(*elt)); - ISC_LINK_INIT(elt, link); - symtab->count++; + isc_result_t result; + elt_t *found = NULL; + elt_t *elt = isc_mem_get(symtab->mctx, sizeof(*elt)); + *elt = (elt_t){ + .key = UNCONST(key), + .size = strlen(key), + .type = type, + .value = value, + }; + uint32_t elt_hashval = elt_hash(elt, symtab->case_sensitive); + isc_hashmap_match_fn elt_match = symtab->case_sensitive + ? elt_match_case + : elt_match_nocase; +again: + result = isc_hashmap_add(symtab->hashmap, elt_hashval, elt_match, elt, + (void *)elt, (void *)&found); + + if (result == ISC_R_SUCCESS) { + SET_IF_NOT_NULL(valuep, elt->value); + return ISC_R_SUCCESS; } - /* - * Though the "key" can be const coming in, it is not stored as const - * so that the calling program can easily have writable access to - * it in its undefine_action function. In the event that it *was* - * truly const coming in and then the caller modified it anyway ... - * well, don't do that! - */ - elt->key = UNCONST(key); - elt->type = type; - elt->value = value; - - /* - * We prepend so that the most recent definition will be found. - */ - PREPEND(symtab->table[bucket], elt, link); - - if (symtab->count > symtab->maxload) { - grow_table(symtab); + switch (exists_policy) { + case isc_symexists_reject: + SET_IF_NOT_NULL(valuep, found->value); + isc_mem_put(symtab->mctx, elt, sizeof(*elt)); + return ISC_R_EXISTS; + case isc_symexists_replace: + result = isc_hashmap_delete(symtab->hashmap, elt_hashval, + elt_match, elt); + INSIST(result == ISC_R_SUCCESS); + elt_destroy(symtab, found); + goto again; + default: + UNREACHABLE(); } return ISC_R_SUCCESS; @@ -257,25 +231,33 @@ isc_symtab_define(isc_symtab_t *symtab, const char *key, unsigned int type, isc_result_t isc_symtab_undefine(isc_symtab_t *symtab, const char *key, unsigned int type) { - unsigned int bucket; - elt_t *elt; - REQUIRE(VALID_SYMTAB(symtab)); REQUIRE(key != NULL); + REQUIRE(type != 0); - FIND(symtab, key, type, bucket, elt); - - if (elt == NULL) { + elt_t *found = NULL; + elt_t elt = { + .key = UNCONST(key), + .size = strlen(key), + .type = type, + }; + uint32_t elt_hashval = elt_hash(&elt, symtab->case_sensitive); + isc_hashmap_match_fn elt_match = symtab->case_sensitive + ? elt_match_case + : elt_match_nocase; + + isc_result_t result = isc_hashmap_find( + symtab->hashmap, elt_hashval, elt_match, &elt, (void **)&found); + + if (result == ISC_R_NOTFOUND) { return ISC_R_NOTFOUND; } - if (symtab->undefine_action != NULL) { - (symtab->undefine_action)(elt->key, elt->type, elt->value, - symtab->undefine_arg); - } - UNLINK(symtab->table[bucket], elt, link); - isc_mem_put(symtab->mctx, elt, sizeof(*elt)); - symtab->count--; + result = isc_hashmap_delete(symtab->hashmap, elt_hashval, elt_match, + &elt); + INSIST(result == ISC_R_SUCCESS); + + elt_destroy(symtab, found); return ISC_R_SUCCESS; } @@ -283,5 +265,30 @@ isc_symtab_undefine(isc_symtab_t *symtab, const char *key, unsigned int type) { unsigned int isc_symtab_count(isc_symtab_t *symtab) { REQUIRE(VALID_SYMTAB(symtab)); - return symtab->count; + + return isc_hashmap_count(symtab->hashmap); +} + +void +isc_symtab_foreach(isc_symtab_t *symtab, isc_symtabforeachaction_t action, + void *arg) { + REQUIRE(VALID_SYMTAB(symtab)); + REQUIRE(action != NULL); + + isc_result_t result; + isc_hashmap_iter_t *it = NULL; + + isc_hashmap_iter_create(symtab->hashmap, &it); + for (result = isc_hashmap_iter_first(it); result == ISC_R_SUCCESS;) { + elt_t *elt = NULL; + isc_hashmap_iter_current(it, (void **)&elt); + if ((action)(elt->key, elt->type, elt->value, arg)) { + elt_destroy(symtab, elt); + result = isc_hashmap_iter_delcurrent_next(it); + } else { + result = isc_hashmap_iter_next(it); + } + } + INSIST(result == ISC_R_NOMORE); + isc_hashmap_iter_destroy(&it); } diff --git a/lib/isccc/Makefile.am b/lib/isccc/Makefile.am index 908b51bcedc..3a81340a48d 100644 --- a/lib/isccc/Makefile.am +++ b/lib/isccc/Makefile.am @@ -9,7 +9,6 @@ libisccc_la_HEADERS = \ include/isccc/cc.h \ include/isccc/ccmsg.h \ include/isccc/sexpr.h \ - include/isccc/symtab.h \ include/isccc/symtype.h \ include/isccc/types.h \ include/isccc/util.h @@ -20,8 +19,7 @@ libisccc_la_SOURCES = \ base64.c \ cc.c \ ccmsg.c \ - sexpr.c \ - symtab.c + sexpr.c libisccc_la_CPPFLAGS = \ $(AM_CPPFLAGS) \ diff --git a/lib/isccc/cc.c b/lib/isccc/cc.c index 4323a54c6b5..fee7f2e22f1 100644 --- a/lib/isccc/cc.c +++ b/lib/isccc/cc.c @@ -40,12 +40,12 @@ #include #include #include +#include #include #include #include #include -#include #include #include @@ -938,8 +938,7 @@ isccc_cc_lookupuint32(isccc_sexpr_t *alist, const char *key, uint32_t *uintp) { } static void -symtab_undefine(char *key, unsigned int type, isccc_symvalue_t value, - void *arg) { +symtab_undefine(char *key, unsigned int type, isc_symvalue_t value, void *arg) { UNUSED(type); UNUSED(value); UNUSED(arg); @@ -948,7 +947,7 @@ symtab_undefine(char *key, unsigned int type, isccc_symvalue_t value, } static bool -symtab_clean(char *key, unsigned int type, isccc_symvalue_t value, void *arg) { +symtab_clean(char *key, unsigned int type, isc_symvalue_t value, void *arg) { isccc_time_t *now; UNUSED(key); @@ -965,15 +964,14 @@ symtab_clean(char *key, unsigned int type, isccc_symvalue_t value, void *arg) { return true; } -isc_result_t -isccc_cc_createsymtab(isccc_symtab_t **symtabp) { - return isccc_symtab_create(11897, symtab_undefine, NULL, false, - symtabp); +void +isccc_cc_createsymtab(isc_mem_t *mctx, isc_symtab_t **symtabp) { + isc_symtab_create(mctx, symtab_undefine, NULL, false, symtabp); } void -isccc_cc_cleansymtab(isccc_symtab_t *symtab, isccc_time_t now) { - isccc_symtab_foreach(symtab, symtab_clean, &now); +isccc_cc_cleansymtab(isc_symtab_t *symtab, isccc_time_t now) { + isc_symtab_foreach(symtab, symtab_clean, &now); } static bool @@ -992,7 +990,7 @@ has_whitespace(const char *str) { } isc_result_t -isccc_cc_checkdup(isccc_symtab_t *symtab, isccc_sexpr_t *message, +isccc_cc_checkdup(isc_symtab_t *symtab, isccc_sexpr_t *message, isccc_time_t now) { const char *_frm; const char *_to; @@ -1000,7 +998,7 @@ isccc_cc_checkdup(isccc_symtab_t *symtab, isccc_sexpr_t *message, isc_result_t result; char *key; size_t len; - isccc_symvalue_t value; + isc_symvalue_t value; isccc_sexpr_t *_ctrl; _ctrl = isccc_alist_lookup(message, "_ctrl"); @@ -1047,8 +1045,8 @@ isccc_cc_checkdup(isccc_symtab_t *symtab, isccc_sexpr_t *message, } snprintf(key, len, "%s;%s;%s;%s", _frm, _to, _ser, _tim); value.as_uinteger = now; - result = isccc_symtab_define(symtab, key, ISCCC_SYMTYPE_CCDUP, value, - isccc_symexists_reject); + result = isc_symtab_define(symtab, key, ISCCC_SYMTYPE_CCDUP, value, + isc_symexists_reject); if (result != ISC_R_SUCCESS) { free(key); return result; diff --git a/lib/isccc/include/isccc/cc.h b/lib/isccc/include/isccc/cc.h index edf89df0417..0e257bde95c 100644 --- a/lib/isccc/include/isccc/cc.h +++ b/lib/isccc/include/isccc/cc.h @@ -35,6 +35,7 @@ #include #include +#include #include @@ -114,14 +115,14 @@ isc_result_t isccc_cc_lookupuint32(isccc_sexpr_t *alist, const char *key, uint32_t *uintp); /*% Create Symbol Table */ -isc_result_t -isccc_cc_createsymtab(isccc_symtab_t **symtabp); +void +isccc_cc_createsymtab(isc_mem_t *mctx, isc_symtab_t **symtabp); /*% Clean up Symbol Table */ void -isccc_cc_cleansymtab(isccc_symtab_t *symtab, isccc_time_t now); +isccc_cc_cleansymtab(isc_symtab_t *symtab, isccc_time_t now); /*% Check for Duplicates */ isc_result_t -isccc_cc_checkdup(isccc_symtab_t *symtab, isccc_sexpr_t *message, +isccc_cc_checkdup(isc_symtab_t *symtab, isccc_sexpr_t *message, isccc_time_t now); diff --git a/lib/isccc/include/isccc/symtab.h b/lib/isccc/include/isccc/symtab.h deleted file mode 100644 index 96dc9429fe1..00000000000 --- a/lib/isccc/include/isccc/symtab.h +++ /dev/null @@ -1,127 +0,0 @@ -/* - * Copyright (C) Internet Systems Consortium, Inc. ("ISC") - * - * SPDX-License-Identifier: MPL-2.0 AND ISC - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * See the COPYRIGHT file distributed with this work for additional - * information regarding copyright ownership. - */ - -/* - * Copyright (C) 2001 Nominum, Inc. - * - * Permission to use, copy, modify, and/or distribute this software for any - * purpose with or without fee is hereby granted, provided that the above - * copyright notice and this permission notice appear in all copies. - * - * THE SOFTWARE IS PROVIDED "AS IS" AND ISC AND NOMINUM DISCLAIMS ALL - * WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES - * OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY - * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES - * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN - * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF - * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. - */ - -#pragma once - -/***** -***** Module Info -*****/ - -/*! \file isccc/symtab.h - * \brief - * Provides a simple memory-based symbol table. - * - * Keys are C strings. A type may be specified when looking up, - * defining, or undefining. A type value of 0 means "match any type"; - * any other value will only match the given type. - * - * It's possible that a client will attempt to define a tuple when a tuple with the given key and type already - * exists in the table. What to do in this case is specified by the - * client. Possible policies are: - * - *\li isccc_symexists_reject Disallow the define, returning #ISC_R_EXISTS - *\li isccc_symexists_replace Replace the old value with the new. The - * undefine action (if provided) will be called - * with the old tuple. - *\li isccc_symexists_add Add the new tuple, leaving the old tuple in - * the table. Subsequent lookups will retrieve - * the most-recently-defined tuple. - * - * A lookup of a key using type 0 will return the most-recently - * defined symbol with that key. An undefine of a key using type 0 - * will undefine the most-recently defined symbol with that key. - * Trying to define a key with type 0 is illegal. - * - * The symbol table library does not make a copy the key field, so the - * caller must ensure that any key it passes to isccc_symtab_define() - * will not change until it calls isccc_symtab_undefine() or - * isccc_symtab_destroy(). - * - * A user-specified action will be called (if provided) when a symbol - * is undefined. It can be used to free memory associated with keys - * and/or values. - */ - -/*** - *** Imports. - ***/ - -#include - -#include - -/*** - *** Symbol Tables. - ***/ - -typedef union isccc_symvalue { - void *as_pointer; - int as_integer; - unsigned int as_uinteger; -} isccc_symvalue_t; - -typedef void (*isccc_symtabundefaction_t)(char *key, unsigned int type, - isccc_symvalue_t value, - void *userarg); - -typedef bool (*isccc_symtabforeachaction_t)(char *key, unsigned int type, - isccc_symvalue_t value, - void *userarg); - -typedef enum { - isccc_symexists_reject = 0, - isccc_symexists_replace = 1, - isccc_symexists_add = 2 -} isccc_symexists_t; - -isc_result_t -isccc_symtab_create(unsigned int size, - isccc_symtabundefaction_t undefine_action, - void *undefine_arg, bool case_sensitive, - isccc_symtab_t **symtabp); - -void -isccc_symtab_destroy(isccc_symtab_t **symtabp); - -isc_result_t -isccc_symtab_lookup(isccc_symtab_t *symtab, const char *key, unsigned int type, - isccc_symvalue_t *value); - -isc_result_t -isccc_symtab_define(isccc_symtab_t *symtab, char *key, unsigned int type, - isccc_symvalue_t value, isccc_symexists_t exists_policy); - -isc_result_t -isccc_symtab_undefine(isccc_symtab_t *symtab, const char *key, - unsigned int type); - -void -isccc_symtab_foreach(isccc_symtab_t *symtab, isccc_symtabforeachaction_t action, - void *arg); diff --git a/lib/isccc/include/isccc/types.h b/lib/isccc/include/isccc/types.h index f2032a76524..c37610ff7c3 100644 --- a/lib/isccc/include/isccc/types.h +++ b/lib/isccc/include/isccc/types.h @@ -42,8 +42,6 @@ typedef uint32_t isccc_time_t; typedef struct isccc_sexpr isccc_sexpr_t; /*% isccc_dottedpair_t typedef */ typedef struct isccc_dottedpair isccc_dottedpair_t; -/*% isccc_symtab_t typedef */ -typedef struct isccc_symtab isccc_symtab_t; /*% iscc region structure */ typedef struct isccc_region { diff --git a/lib/isccfg/check.c b/lib/isccfg/check.c index 68dd6dd6cd2..c60eec4b676 100644 --- a/lib/isccfg/check.c +++ b/lib/isccfg/check.c @@ -13,7 +13,6 @@ /*! \file */ -#include #include #include #include @@ -469,7 +468,7 @@ exists(const cfg_obj_t *obj, const char *name, int value, isc_symtab_t *symtab, static isc_result_t checkacl(const char *aclname, cfg_aclconfctx_t *actx, const cfg_obj_t *zconfig, const cfg_obj_t *voptions, const cfg_obj_t *config, isc_mem_t *mctx) { - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; const cfg_obj_t *aclobj = NULL; const cfg_obj_t *options; dns_acl_t *acl = NULL; @@ -1003,7 +1002,7 @@ kasp_name_allowed(const cfg_listelt_t *element) { static const cfg_obj_t * find_maplist(const cfg_obj_t *config, const char *listname, const char *name) { - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; const cfg_obj_t *maplist = NULL; const cfg_listelt_t *elt = NULL; @@ -1166,7 +1165,7 @@ check_listeners(const cfg_obj_t *list, const cfg_obj_t *config, static isc_result_t check_port(const cfg_obj_t *options, const char *type, in_port_t *portp) { const cfg_obj_t *portobj = NULL; - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; result = cfg_map_get(options, type, &portobj); if (result != ISC_R_SUCCESS) { @@ -2049,7 +2048,7 @@ static isc_result_t check_remoteserverlist(const cfg_obj_t *cctx, const char *list, isc_symtab_t *symtab, isc_mem_t *mctx) { isc_symvalue_t symvalue; - isc_result_t result, tresult; + isc_result_t result = ISC_R_SUCCESS, tresult; const cfg_obj_t *obj = NULL; const cfg_listelt_t *elt; @@ -2105,7 +2104,7 @@ check_remoteserverlists(const cfg_obj_t *cctx, isc_mem_t *mctx) { isc_result_t result = ISC_R_SUCCESS, tresult; isc_symtab_t *symtab = NULL; - isc_symtab_create(mctx, 100, freekey, mctx, false, &symtab); + isc_symtab_create(mctx, freekey, mctx, false, &symtab); tresult = check_remoteserverlist(cctx, "remote-servers", symtab, mctx); if (tresult != ISC_R_SUCCESS) { @@ -2131,7 +2130,7 @@ check_remoteserverlists(const cfg_obj_t *cctx, isc_mem_t *mctx) { #if HAVE_LIBNGHTTP2 static isc_result_t check_httpserver(const cfg_obj_t *http, isc_symtab_t *symtab) { - isc_result_t result, tresult; + isc_result_t result = ISC_R_SUCCESS, tresult; const char *name = cfg_obj_asstring(cfg_map_getname(http)); const cfg_obj_t *eps = NULL; const cfg_listelt_t *elt = NULL; @@ -2193,12 +2192,12 @@ check_httpserver(const cfg_obj_t *http, isc_symtab_t *symtab) { static isc_result_t check_httpservers(const cfg_obj_t *config, isc_mem_t *mctx) { - isc_result_t result, tresult; + isc_result_t result = ISC_R_SUCCESS, tresult; const cfg_obj_t *obj = NULL; const cfg_listelt_t *elt = NULL; isc_symtab_t *symtab = NULL; - isc_symtab_create(mctx, 100, NULL, NULL, false, &symtab); + isc_symtab_create(mctx, NULL, NULL, false, &symtab); result = cfg_map_get(config, "http", &obj); if (result != ISC_R_SUCCESS) { @@ -2223,7 +2222,7 @@ done: static isc_result_t check_tls_defintion(const cfg_obj_t *tlsobj, const char *name, isc_symtab_t *symtab) { - isc_result_t result, tresult; + isc_result_t result = ISC_R_SUCCESS, tresult; const cfg_obj_t *tls_proto_list = NULL, *tls_key = NULL, *tls_cert = NULL, *tls_ciphers = NULL, *tls_cipher_suites = NULL; @@ -2357,7 +2356,7 @@ check_tls_defintion(const cfg_obj_t *tlsobj, const char *name, static isc_result_t check_tls_definitions(const cfg_obj_t *config, isc_mem_t *mctx) { - isc_result_t result, tresult; + isc_result_t result = ISC_R_SUCCESS, tresult; const cfg_obj_t *obj = NULL; const cfg_listelt_t *elt = NULL; isc_symtab_t *symtab = NULL; @@ -2368,7 +2367,7 @@ check_tls_definitions(const cfg_obj_t *config, isc_mem_t *mctx) { return result; } - isc_symtab_create(mctx, 100, NULL, NULL, false, &symtab); + isc_symtab_create(mctx, NULL, NULL, false, &symtab); for (elt = cfg_list_first(obj); elt != NULL; elt = cfg_list_next(elt)) { const char *name; @@ -2388,7 +2387,7 @@ check_tls_definitions(const cfg_obj_t *config, isc_mem_t *mctx) { static isc_result_t get_remotes(const cfg_obj_t *cctx, const char *list, const char *name, const cfg_obj_t **ret) { - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; const cfg_obj_t *obj = NULL; const cfg_listelt_t *elt = NULL; @@ -2449,7 +2448,7 @@ validate_remotes(const cfg_obj_t *obj, const cfg_obj_t *config, const cfg_obj_t *listobj; REQUIRE(countp != NULL); - isc_symtab_create(mctx, 100, NULL, NULL, false, &symtab); + isc_symtab_create(mctx, NULL, NULL, false, &symtab); newlist: listobj = cfg_tuple_get(obj, "addresses"); @@ -2833,7 +2832,7 @@ check_recursion(const cfg_obj_t *config, const cfg_obj_t *voptions, isc_mem_t *mctx) { dns_acl_t *acl = NULL; const cfg_obj_t *obj; - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; bool retval = true; /* @@ -3175,7 +3174,7 @@ check_zoneconf(const cfg_obj_t *zconfig, const cfg_obj_t *voptions, RUNTIME_CHECK(n > 0 && (size_t)n < len); switch (ztype) { case CFG_ZONE_INVIEW: - tresult = isc_symtab_lookup(inview, key, 0, NULL); + tresult = isc_symtab_lookup(inview, key, 1, NULL); if (tresult != ISC_R_SUCCESS) { cfg_obj_log(inviewobj, ISC_LOG_ERROR, "'in-view' zone '%s' " @@ -3913,7 +3912,7 @@ isccfg_check_key(const cfg_obj_t *key) { const char *algorithm; int i; size_t len = 0; - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; isc_buffer_t buf; unsigned char secretbuf[1024]; static const algorithmtable algorithms[] = { @@ -4003,50 +4002,67 @@ isccfg_check_key(const cfg_obj_t *key) { return ISC_R_SUCCESS; } +typedef enum symtab_file_type { + READ_ONLY = 1, + WRITEABLE = 2, +} symtab_file_type_t; + static isc_result_t fileexist(const cfg_obj_t *obj, isc_symtab_t *symtab, bool writeable) { - isc_result_t result; - isc_symvalue_t symvalue; + isc_result_t result_ro, result_w; + isc_symvalue_t symvalue_ro, symvalue_w; unsigned int line; const char *file; - result = isc_symtab_lookup(symtab, cfg_obj_asstring(obj), 0, &symvalue); - if (result == ISC_R_SUCCESS) { - if (writeable) { - file = cfg_obj_file(symvalue.as_cpointer); - line = cfg_obj_line(symvalue.as_cpointer); - cfg_obj_log(obj, ISC_LOG_ERROR, - "writeable file '%s': already in use: " - "%s:%u", - cfg_obj_asstring(obj), file, line); - return ISC_R_EXISTS; - } - result = isc_symtab_lookup(symtab, cfg_obj_asstring(obj), 2, - &symvalue); - if (result == ISC_R_SUCCESS) { - file = cfg_obj_file(symvalue.as_cpointer); - line = cfg_obj_line(symvalue.as_cpointer); - cfg_obj_log(obj, ISC_LOG_ERROR, - "writeable file '%s': already in use: " - "%s:%u", - cfg_obj_asstring(obj), file, line); - return ISC_R_EXISTS; - } + /* + * Since symtab doesn't let us query the file type, we need to query + * twice. Once per type. + */ + result_ro = isc_symtab_lookup(symtab, cfg_obj_asstring(obj), READ_ONLY, + &symvalue_ro); + result_w = isc_symtab_lookup(symtab, cfg_obj_asstring(obj), WRITEABLE, + &symvalue_w); + + bool found_read_only = result_ro == ISC_R_SUCCESS; + bool found_writable = result_w == ISC_R_SUCCESS; + + /* + * If either the new file, the old file or both files are writeable, + * bail out. + */ + if ((writeable && found_read_only) || found_writable) { + isc_symvalue_t symvalue = (writeable && found_read_only) + ? symvalue_ro + : symvalue_w; + + file = cfg_obj_file(symvalue.as_cpointer); + line = cfg_obj_line(symvalue.as_cpointer); + cfg_obj_log(obj, ISC_LOG_ERROR, + "writeable file '%s': already in use: " + "%s:%u", + cfg_obj_asstring(obj), file, line); + return ISC_R_EXISTS; + } else if (found_read_only) { + /* If a read only file is already in the hashtable, we have + * nothing to do */ return ISC_R_SUCCESS; + } else { + /* If the file was not present already in the hashmap, add it */ + isc_symvalue_t symvalue = + (isc_symvalue_t){ .as_cpointer = obj }; + symtab_file_type_t type = writeable ? WRITEABLE : READ_ONLY; + isc_result_t result = + isc_symtab_define(symtab, cfg_obj_asstring(obj), type, + symvalue, isc_symexists_reject); + return result; } - - symvalue.as_cpointer = obj; - result = isc_symtab_define(symtab, cfg_obj_asstring(obj), - writeable ? 2 : 1, symvalue, - isc_symexists_reject); - return result; } static isc_result_t keydirexist(const cfg_obj_t *zcfg, const char *optname, dns_name_t *zname, const char *dirname, const char *kaspnamestr, isc_symtab_t *symtab, isc_mem_t *mctx) { - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; isc_symvalue_t symvalue; char *symkey; char keydirbuf[DNS_NAME_FORMATSIZE + 128]; @@ -4070,7 +4086,7 @@ keydirexist(const cfg_obj_t *zcfg, const char *optname, dns_name_t *zname, } keydir = keydirbuf; - result = isc_symtab_lookup(symtab, keydir, 0, &symvalue); + result = isc_symtab_lookup(symtab, keydir, 1, &symvalue); if (result == ISC_R_SUCCESS) { const cfg_obj_t *kasp = NULL; const cfg_obj_t *exist = symvalue.as_cpointer; @@ -4104,7 +4120,7 @@ keydirexist(const cfg_obj_t *zcfg, const char *optname, dns_name_t *zname, */ symkey = isc_mem_strdup(mctx, keydir); symvalue.as_cpointer = zcfg; - result = isc_symtab_define(symtab, symkey, 2, symvalue, + result = isc_symtab_define(symtab, symkey, 1, symvalue, isc_symexists_reject); RUNTIME_CHECK(result == ISC_R_SUCCESS); return result; @@ -4888,8 +4904,8 @@ check_ta_conflicts(const cfg_obj_t *global_ta, const cfg_obj_t *view_ta, const cfg_obj_t *keylist = NULL; isc_symtab_t *statictab = NULL, *dstab = NULL; - isc_symtab_create(mctx, 100, freekey, mctx, false, &statictab); - isc_symtab_create(mctx, 100, freekey, mctx, false, &dstab); + isc_symtab_create(mctx, freekey, mctx, false, &statictab); + isc_symtab_create(mctx, freekey, mctx, false, &dstab); /* * First we record all the static keys (trust-anchors configured with @@ -4971,7 +4987,7 @@ check_rpz_catz(const char *rpz_catz, const cfg_obj_t *rpz_obj, const char *zonename, *zonetype; const char *forview = " for view "; isc_symvalue_t value; - isc_result_t result, tresult; + isc_result_t result = ISC_R_SUCCESS, tresult; dns_fixedname_t fixed; dns_name_t *name; char namebuf[DNS_NAME_FORMATSIZE]; @@ -5106,7 +5122,7 @@ check_catz(const cfg_obj_t *catz_obj, const char *viewname, isc_mem_t *mctx) { forview = ""; } - isc_symtab_create(mctx, 100, freekey, mctx, false, &symtab); + isc_symtab_create(mctx, freekey, mctx, false, &symtab); obj = cfg_tuple_get(catz_obj, "zone list"); @@ -5186,7 +5202,7 @@ check_one_plugin(const cfg_obj_t *config, const cfg_obj_t *obj, void *callback_data) { struct check_one_plugin_data *data = callback_data; char full_path[PATH_MAX]; - isc_result_t result; + isc_result_t result = ISC_R_SUCCESS; result = ns_plugin_expandpath(plugin_path, full_path, sizeof(full_path)); @@ -5287,7 +5303,7 @@ check_viewconf(const cfg_obj_t *config, const cfg_obj_t *voptions, * Check that all zone statements are syntactically correct and * there are no duplicate zones. */ - isc_symtab_create(mctx, 1000, freekey, mctx, false, &symtab); + isc_symtab_create(mctx, freekey, mctx, false, &symtab); cfg_aclconfctx_create(mctx, &actx); @@ -5400,7 +5416,7 @@ check_viewconf(const cfg_obj_t *config, const cfg_obj_t *voptions, * Check that all key statements are syntactically correct and * there are no duplicate keys. */ - isc_symtab_create(mctx, 1000, freekey, mctx, false, &symtab); + isc_symtab_create(mctx, freekey, mctx, false, &symtab); (void)cfg_map_get(config, "key", &keys); tresult = check_keylist(keys, symtab, mctx); @@ -5630,7 +5646,7 @@ check_logging(const cfg_obj_t *config, isc_mem_t *mctx) { return ISC_R_SUCCESS; } - isc_symtab_create(mctx, 100, NULL, NULL, false, &symtab); + isc_symtab_create(mctx, NULL, NULL, false, &symtab); symvalue.as_cpointer = NULL; for (i = 0; default_channels[i] != NULL; i++) { @@ -5764,7 +5780,7 @@ check_controls(const cfg_obj_t *config, isc_mem_t *mctx) { cfg_aclconfctx_create(mctx, &actx); - isc_symtab_create(mctx, 100, freekey, mctx, true, &symtab); + isc_symtab_create(mctx, freekey, mctx, true, &symtab); /* * INET: Check allow clause. @@ -5839,7 +5855,7 @@ isccfg_check_namedconf(const cfg_obj_t *config, unsigned int flags, const cfg_obj_t *acls = NULL; const cfg_listelt_t *velement; isc_result_t result = ISC_R_SUCCESS; - isc_result_t tresult; + isc_result_t tresult = ISC_R_SUCCESS; isc_symtab_t *symtab = NULL; isc_symtab_t *files = NULL; isc_symtab_t *keydirs = NULL; @@ -5900,9 +5916,9 @@ isccfg_check_namedconf(const cfg_obj_t *config, unsigned int flags, * case sensitive. This will prevent people using FOO.DB and foo.db * on case sensitive file systems but that shouldn't be a major issue. */ - isc_symtab_create(mctx, 100, NULL, NULL, false, &files); - isc_symtab_create(mctx, 100, freekey, mctx, false, &keydirs); - isc_symtab_create(mctx, 100, freekey, mctx, true, &inview); + isc_symtab_create(mctx, NULL, NULL, false, &files); + isc_symtab_create(mctx, freekey, mctx, false, &keydirs); + isc_symtab_create(mctx, freekey, mctx, true, &inview); if (views == NULL) { tresult = check_viewconf(config, NULL, NULL, dns_rdataclass_in, @@ -5931,7 +5947,7 @@ isccfg_check_namedconf(const cfg_obj_t *config, unsigned int flags, } } - isc_symtab_create(mctx, 100, NULL, NULL, true, &symtab); + isc_symtab_create(mctx, NULL, NULL, true, &symtab); for (velement = cfg_list_first(views); velement != NULL; velement = cfg_list_next(velement)) diff --git a/lib/isccfg/parser.c b/lib/isccfg/parser.c index 7571d8cb346..44bb83c662d 100644 --- a/lib/isccfg/parser.c +++ b/lib/isccfg/parser.c @@ -72,7 +72,9 @@ #define CAT CFG_LOGCATEGORY_CONFIG #define MOD CFG_LOGMODULE_PARSER -#define MAP_SYM 1 /* Unique type for isc_symtab */ +/* isc_symtab_t takes both a string and an int as input, but we don't need + * the int, so we define a "dummy" value to use instead. */ +#define SYMTAB_DUMMY_TYPE 1 #define TOKEN_STRING(pctx) (pctx->token.value.as_textregion.base) #define TOKEN_REGION(pctx) (pctx->token.value.as_textregion) @@ -2207,7 +2209,6 @@ cfg_parse_mapbody(cfg_parser_t *pctx, const cfg_type_t *type, cfg_obj_t **ret) { cfg_obj_t *eltobj = NULL; cfg_obj_t *includename = NULL; isc_symvalue_t symval; - cfg_list_t *list = NULL; REQUIRE(pctx != NULL); REQUIRE(type != NULL); @@ -2358,32 +2359,20 @@ cfg_parse_mapbody(cfg_parser_t *pctx, const cfg_type_t *type, cfg_obj_t **ret) { } /* See if the clause already has a value; if not create one. */ - result = isc_symtab_lookup(obj->value.map.symtab, clause->name, - 0, &symval); - if ((clause->flags & CFG_CLAUSEFLAG_MULTI) != 0) { /* Multivalued clause */ cfg_obj_t *listobj = NULL; - if (result == ISC_R_NOTFOUND) { - CHECK(cfg_create_list(pctx, - &cfg_type_implicitlist, - &listobj)); - symval.as_pointer = listobj; - result = isc_symtab_define( - obj->value.map.symtab, clause->name, 1, - symval, isc_symexists_reject); - if (result != ISC_R_SUCCESS) { - cfg_parser_error(pctx, CFG_LOG_NEAR, - "isc_symtab_define(%s)" - " " - "failed", - clause->name); - isc_mem_put(pctx->mctx, list, - sizeof(cfg_list_t)); - goto cleanup; - } - } else { - INSIST(result == ISC_R_SUCCESS); + + CHECK(cfg_create_list(pctx, &cfg_type_implicitlist, + &listobj)); + symval.as_pointer = listobj; + result = isc_symtab_define_and_return( + obj->value.map.symtab, clause->name, + SYMTAB_DUMMY_TYPE, symval, isc_symexists_reject, + &symval); + + if (result == ISC_R_EXISTS) { + CLEANUP_OBJ(listobj); listobj = symval.as_pointer; } @@ -2394,25 +2383,23 @@ cfg_parse_mapbody(cfg_parser_t *pctx, const cfg_type_t *type, cfg_obj_t **ret) { ISC_LIST_APPEND(listobj->value.list, elt, link); } else { /* Single-valued clause */ - if (result == ISC_R_NOTFOUND) { - bool callback = ((clause->flags & - CFG_CLAUSEFLAG_CALLBACK) != - 0); - CHECK(parse_symtab_elt( - pctx, clause->name, clause->type, - obj->value.map.symtab, callback)); - CHECK(parse_semicolon(pctx)); - } else if (result == ISC_R_SUCCESS) { + bool callback = ((clause->flags & + CFG_CLAUSEFLAG_CALLBACK) != 0); + + result = parse_symtab_elt( + pctx, clause->name, clause->type, + obj->value.map.symtab, callback); + if (result == ISC_R_EXISTS) { cfg_parser_error(pctx, CFG_LOG_NEAR, "'%s' redefined", clause->name); - result = ISC_R_EXISTS; - goto cleanup; - } else { + CHECK(result); + } else if (result != ISC_R_SUCCESS) { cfg_parser_error(pctx, CFG_LOG_NEAR, "isc_symtab_define() failed"); - goto cleanup; + CHECK(result); } + CHECK(parse_semicolon(pctx)); } } @@ -2441,7 +2428,8 @@ parse_symtab_elt(cfg_parser_t *pctx, const char *name, cfg_type_t *elttype, } symval.as_pointer = obj; - CHECK(isc_symtab_define(symtab, name, 1, symval, isc_symexists_reject)); + CHECK(isc_symtab_define(symtab, name, SYMTAB_DUMMY_TYPE, symval, + isc_symexists_reject)); return ISC_R_SUCCESS; cleanup: @@ -2556,7 +2544,8 @@ cfg_print_mapbody(cfg_printer_t *pctx, const cfg_obj_t *obj) { for (clause = *clauseset; clause->name != NULL; clause++) { isc_result_t result; result = isc_symtab_lookup(obj->value.map.symtab, - clause->name, 0, &symval); + clause->name, + SYMTAB_DUMMY_TYPE, &symval); if (result == ISC_R_SUCCESS) { cfg_obj_t *symobj = symval.as_pointer; if (symobj->type == &cfg_type_implicitlist) { @@ -2727,7 +2716,7 @@ cfg_map_get(const cfg_obj_t *mapobj, const char *name, const cfg_obj_t **obj) { map = &mapobj->value.map; - result = isc_symtab_lookup(map->symtab, name, MAP_SYM, &val); + result = isc_symtab_lookup(map->symtab, name, SYMTAB_DUMMY_TYPE, &val); if (result != ISC_R_SUCCESS) { return result; } @@ -3797,8 +3786,8 @@ create_map(cfg_parser_t *pctx, const cfg_type_t *type, cfg_obj_t **ret) { cfg_obj_t *obj = NULL; CHECK(cfg_create_obj(pctx, type, &obj)); - isc_symtab_create(pctx->mctx, 5, /* XXX */ - map_symtabitem_destroy, pctx, false, &symtab); + isc_symtab_create(pctx->mctx, map_symtabitem_destroy, pctx, false, + &symtab); obj->value.map.symtab = symtab; obj->value.map.id = NULL; @@ -3922,7 +3911,8 @@ breakout: return ISC_R_FAILURE; } - result = isc_symtab_lookup(map->symtab, clausename, 0, &symval); + result = isc_symtab_lookup(map->symtab, clausename, SYMTAB_DUMMY_TYPE, + &symval); if (result == ISC_R_NOTFOUND) { if ((clause->flags & CFG_CLAUSEFLAG_MULTI) != 0) { CHECK(cfg_create_list(pctx, &cfg_type_implicitlist, @@ -3935,8 +3925,10 @@ breakout: symval.as_pointer = obj; } - CHECK(isc_symtab_define(map->symtab, clausename, 1, symval, - isc_symexists_reject)); + result = isc_symtab_define(map->symtab, clausename, + SYMTAB_DUMMY_TYPE, symval, + isc_symexists_reject); + INSIST(result == ISC_R_SUCCESS); } else { cfg_obj_t *destobj2 = symval.as_pointer; diff --git a/tests/isc/symtab_test.c b/tests/isc/symtab_test.c index b7a15400752..24da351bc84 100644 --- a/tests/isc/symtab_test.c +++ b/tests/isc/symtab_test.c @@ -28,105 +28,254 @@ #include -static void -undefine(char *key, unsigned int type, isc_symvalue_t value, void *arg) { - UNUSED(arg); +#define TEST_NITEMS 10000 - assert_int_equal(type, 1); +static void +undefine(char *key, unsigned int type ISC_ATTR_UNUSED, isc_symvalue_t value, + void *arg ISC_ATTR_UNUSED) { isc_mem_free(mctx, key); isc_mem_free(mctx, value.as_pointer); } -/* test symbol table growth */ -ISC_RUN_TEST_IMPL(symtab_grow) { +ISC_RUN_TEST_IMPL(symtab_define) { isc_result_t result; - isc_symtab_t *st = NULL; + isc_symtab_t *symtab = NULL; isc_symvalue_t value; + isc_symvalue_t found; isc_symexists_t policy = isc_symexists_reject; - int i; + char str[16], *key; + snprintf(str, sizeof(str), "%p", "define"); + key = isc_mem_strdup(mctx, str); - UNUSED(state); + isc_symtab_create(mctx, undefine, NULL, false, &symtab); + assert_non_null(symtab); - isc_symtab_create(mctx, 3, undefine, NULL, false, &st); - assert_non_null(st); + value.as_pointer = isc_mem_strdup(mctx, key); + assert_non_null(value.as_pointer); - /* Nothing should be in the table yet */ + result = isc_symtab_define(symtab, key, 1, value, policy); + assert_int_equal(result, ISC_R_SUCCESS); - /* - * Put 1024 entries in the table (this should necessate - * regrowing the hash table several times + result = isc_symtab_lookup(symtab, key, 1, &found); + assert_int_equal(result, ISC_R_SUCCESS); + assert_string_equal(value.as_pointer, found.as_pointer); + + result = isc_symtab_lookup(symtab, key, 2, NULL); + assert_int_equal(result, ISC_R_NOTFOUND); + + isc_symtab_destroy(&symtab); +} + +ISC_RUN_TEST_IMPL(symtab_undefine) { + isc_result_t result; + isc_symtab_t *symtab = NULL; + isc_symvalue_t value; + isc_symexists_t policy = isc_symexists_reject; + + /* We need a separate copy of the key to prevent an use-after-free */ + char str[16], *key, *key_after_undefine; + snprintf(str, sizeof(str), "%p", "undefine"); + + key = isc_mem_strdup(mctx, str); + key_after_undefine = isc_mem_strdup(mctx, str); + + isc_symtab_create(mctx, undefine, NULL, false, &symtab); + assert_non_null(symtab); + + value.as_pointer = isc_mem_strdup(mctx, key); + assert_non_null(value.as_pointer); + + result = isc_symtab_define(symtab, key, 1, value, policy); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_symtab_lookup(symtab, key_after_undefine, 1, NULL); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_symtab_undefine(symtab, key, 1); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_symtab_lookup(symtab, key_after_undefine, 1, NULL); + assert_int_equal(result, ISC_R_NOTFOUND); + + isc_symtab_destroy(&symtab); + + /* key will be freed by isc_symtab_undefine, so we don't need to free + * it again */ - for (i = 0; i < 1024; i++) { - char str[16], *key; + isc_mem_free(mctx, key_after_undefine); +} - snprintf(str, sizeof(str), "%04x", i); - key = isc_mem_strdup(mctx, str); - assert_non_null(key); - value.as_pointer = isc_mem_strdup(mctx, str); - assert_non_null(value.as_pointer); - result = isc_symtab_define(st, key, 1, value, policy); - assert_int_equal(result, ISC_R_SUCCESS); - if (result != ISC_R_SUCCESS) { - undefine(key, 1, value, NULL); - } - } +ISC_RUN_TEST_IMPL(symtab_replace) { + isc_result_t result; + isc_symtab_t *symtab = NULL; + isc_symvalue_t value1; + isc_symvalue_t value2; + isc_symvalue_t found; + isc_symexists_t policy = isc_symexists_replace; + char str[16], *key1, *key2; + snprintf(str, sizeof(str), "%p", "replace"); + key1 = isc_mem_strdup(mctx, str); + key2 = isc_mem_strdup(mctx, str); + + isc_symtab_create(mctx, undefine, NULL, false, &symtab); + assert_non_null(symtab); + + value1.as_pointer = isc_mem_strdup(mctx, key1); + assert_non_null(value1.as_pointer); + + value2.as_pointer = isc_mem_strdup(mctx, key2); + assert_non_null(value2.as_pointer); + + result = isc_symtab_define(symtab, key1, 1, value1, policy); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_symtab_lookup(symtab, key1, 1, &found); + assert_int_equal(result, ISC_R_SUCCESS); + assert_string_equal(value1.as_pointer, found.as_pointer); + + result = isc_symtab_define(symtab, key2, 1, value2, policy); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_symtab_lookup(symtab, key2, 1, &found); + assert_int_equal(result, ISC_R_SUCCESS); + assert_string_equal(value2.as_pointer, found.as_pointer); + + result = isc_symtab_undefine(symtab, key2, 1); + assert_int_equal(result, ISC_R_SUCCESS); + + isc_symtab_destroy(&symtab); +} + +ISC_RUN_TEST_IMPL(symtab_reject) { + isc_result_t result; + isc_symtab_t *symtab = NULL; + isc_symvalue_t value1; + isc_symvalue_t value2; + isc_symvalue_t found; + isc_symexists_t policy = isc_symexists_reject; + char str[16], *key1, *key2; + snprintf(str, sizeof(str), "%p", "reject"); + key1 = isc_mem_strdup(mctx, str); + key2 = isc_mem_strdup(mctx, str); + + isc_symtab_create(mctx, undefine, NULL, false, &symtab); + assert_non_null(symtab); + + value1.as_pointer = isc_mem_strdup(mctx, key1); + assert_non_null(value1.as_pointer); + + value2.as_pointer = isc_mem_strdup(mctx, key2); + assert_non_null(value2.as_pointer); + + result = isc_symtab_define(symtab, key1, 1, value1, policy); + assert_int_equal(result, ISC_R_SUCCESS); + + result = isc_symtab_lookup(symtab, key1, 1, &found); + assert_int_equal(result, ISC_R_SUCCESS); + assert_string_equal(value1.as_pointer, found.as_pointer); + + result = isc_symtab_define_and_return(symtab, key2, 1, value2, policy, + &found); + assert_int_equal(result, ISC_R_EXISTS); + assert_string_equal(value1.as_pointer, found.as_pointer); + + result = isc_symtab_lookup(symtab, key2, 1, &found); + assert_int_equal(result, ISC_R_SUCCESS); + assert_string_equal(value1.as_pointer, found.as_pointer); + + result = isc_symtab_undefine(symtab, key1, 1); + assert_int_equal(result, ISC_R_SUCCESS); + + undefine(key2, 1, value2, NULL); + + isc_symtab_destroy(&symtab); +} + +static bool +peek(char *key ISC_ATTR_UNUSED, unsigned int type, + isc_symvalue_t value ISC_ATTR_UNUSED, void *arg) { + bool *seen = arg; + size_t i = type - 1; + + assert_false(seen[i]); + + seen[i] = true; + + return i % 2; +} + +ISC_RUN_TEST_IMPL(symtab_foreach) { + isc_result_t result; + isc_symtab_t *symtab = NULL; + isc_symvalue_t value; + isc_symexists_t policy = isc_symexists_reject; + bool seen[TEST_NITEMS] = { 0 }; + + isc_symtab_create(mctx, undefine, NULL, false, &symtab); + + /* Nothing should be in the table yet */ + assert_non_null(symtab); /* - * Try to put them in again; this should fail + * Put TEST_NITEMS entries in the table. */ - for (i = 0; i < 1024; i++) { - char str[16], *key; + for (size_t i = 0; i < TEST_NITEMS; i++) { + char str[256] = {}, *key; + + snprintf(str, sizeof(str), "%08zx", i); - snprintf(str, sizeof(str), "%04x", i); key = isc_mem_strdup(mctx, str); assert_non_null(key); value.as_pointer = isc_mem_strdup(mctx, str); assert_non_null(value.as_pointer); - result = isc_symtab_define(st, key, 1, value, policy); - assert_int_equal(result, ISC_R_EXISTS); - undefine(key, 1, value, NULL); + result = isc_symtab_define(symtab, key, i + 1, value, policy); + assert_int_equal(result, ISC_R_SUCCESS); } /* * Retrieve them; this should succeed */ - for (i = 0; i < 1024; i++) { - char str[16]; + for (size_t i = 0; i < TEST_NITEMS; i++) { + char str[256] = {}; - snprintf(str, sizeof(str), "%04x", i); - result = isc_symtab_lookup(st, str, 0, &value); + snprintf(str, sizeof(str), "%08zx", i); + result = isc_symtab_lookup(symtab, str, i + 1, &value); assert_int_equal(result, ISC_R_SUCCESS); assert_string_equal(str, (char *)value.as_pointer); } /* - * Undefine them + * Undefine even items them via foreach */ - for (i = 0; i < 1024; i++) { - char str[16]; + isc_symtab_foreach(symtab, peek, seen); - snprintf(str, sizeof(str), "%04x", i); - result = isc_symtab_undefine(st, str, 1); - assert_int_equal(result, ISC_R_SUCCESS); + for (size_t i = 0; i < TEST_NITEMS; i++) { + assert_true(seen[i]); } /* - * Retrieve them again; this should fail + * Destroy the even ones by hand. */ - for (i = 0; i < 1024; i++) { - char str[16]; + for (size_t i = 0; i < TEST_NITEMS; i++) { + if (i % 2 == 0) { + char str[256] = {}; - snprintf(str, sizeof(str), "%04x", i); - result = isc_symtab_lookup(st, str, 0, &value); - assert_int_equal(result, ISC_R_NOTFOUND); + snprintf(str, sizeof(str), "%08zx", i); + result = isc_symtab_undefine(symtab, str, i + 1); + assert_int_equal(result, ISC_R_SUCCESS); + } } - isc_symtab_destroy(&st); + isc_symtab_destroy(&symtab); } ISC_TEST_LIST_START -ISC_TEST_ENTRY(symtab_grow) +ISC_TEST_ENTRY(symtab_define) +ISC_TEST_ENTRY(symtab_undefine) +ISC_TEST_ENTRY(symtab_reject) +ISC_TEST_ENTRY(symtab_replace) +ISC_TEST_ENTRY(symtab_foreach) ISC_TEST_LIST_END