From 614ce1b65ff9ef158aa5a19a9acf2edb99170963 Mon Sep 17 00:00:00 2001 From: Mukund Sivaraman Date: Mon, 8 Feb 2016 14:51:53 +0530 Subject: [PATCH] Add tests for hash function, and comment dns_rbt_addnode() (#41179) No CHANGES entry necessary. --- lib/dns/rbt.c | 55 +++++++++++++++----- lib/isc/hash.c | 8 +++ lib/isc/tests/hash_test.c | 107 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 154 insertions(+), 16 deletions(-) diff --git a/lib/dns/rbt.c b/lib/dns/rbt.c index 9fb08075f0..ef02f200b1 100644 --- a/lib/dns/rbt.c +++ b/lib/dns/rbt.c @@ -1128,6 +1128,37 @@ dns_rbt_addnode(dns_rbt_t *rbt, dns_name_t *name, dns_rbtnode_t **nodep) { REQUIRE(dns_name_isabsolute(name)); REQUIRE(nodep != NULL && *nodep == NULL); + /* + * Dear future BIND developer, + * + * After you have tried attempting to optimize this routine by + * using the hashtable and have realized your folly, please + * append another cross ("X") below as a warning to the next + * future BIND developer: + * + * Number of victim developers: X + * + * I wish the past developer had included such a notice. + * + * Long form: Unlike dns_rbt_findnode(), this function does not + * lend itself to be optimized using the hashtable: + * + * 1. In the subtree where the insertion occurs, this function + * needs to have the insertion point and the order where the + * lookup terminated (i.e., at the insertion point where left or + * right child is NULL). This cannot be determined from the + * hashtable, so at least in that subtree, a BST O(log N) lookup + * is necessary. + * + * 2. Our RBT nodes contain not only single labels but label + * sequences to optimize space usage. So at every level, we have + * to look for a match in the hashtable for all superdomains in + * the rest of the name we're searching. This is an O(N) + * operation at least, here N being the label size of name, each + * of which is a hashtable lookup involving dns_name_equal() + * comparisons. + */ + /* * Create a copy of the name so the original name structure is * not modified. @@ -1437,7 +1468,7 @@ dns_rbt_findnode(dns_rbt_t *rbt, dns_name_t *name, dns_name_t *foundname, unsigned int options, dns_rbtfindcallback_t callback, void *callback_arg) { - dns_rbtnode_t *current, *last_compared, *current_root; + dns_rbtnode_t *current, *last_compared; dns_rbtnodechain_t localchain; dns_name_t *search_name, current_name, *callback_name; dns_fixedname_t fixedcallbackname, fixedsearchname; @@ -1494,7 +1525,6 @@ dns_rbt_findnode(dns_rbt_t *rbt, dns_name_t *name, dns_name_t *foundname, saved_result = ISC_R_SUCCESS; current = rbt->root; - current_root = rbt->root; while (ISC_LIKELY(current != NULL)) { NODENAME(current, ¤t_name); @@ -1531,22 +1561,23 @@ dns_rbt_findnode(dns_rbt_t *rbt, dns_name_t *name, dns_name_t *foundname, unsigned int hash; /* - * The case of current != current_root, that - * means a left or right pointer was followed, - * only happens when the algorithm fell through to - * the traditional binary search because of a - * bitstring label. Since we dropped the bitstring - * support, this should not happen. + * The case of current not being a subtree root, + * that means a left or right pointer was + * followed, only happens when the algorithm + * fell through to the traditional binary search + * because of a bitstring label. Since we + * dropped the bitstring support, this should + * not happen. */ - INSIST(current == current_root); + INSIST(IS_ROOT(current)); nlabels = dns_name_countlabels(search_name); /* - * current_root is the root of the current level, so + * current is the root of the current level, so * its parent is the same as its "up" pointer. */ - up_current = PARENT(current_root); + up_current = PARENT(current); dns_name_init(&hash_name, NULL); hashagain: @@ -1714,8 +1745,6 @@ dns_rbt_findnode(dns_rbt_t *rbt, dns_name_t *name, dns_name_t *foundname, * Finally, head to the next tree level. */ current = DOWN(current); - current_root = current; - } else { /* * Though there are labels in common, the diff --git a/lib/isc/hash.c b/lib/isc/hash.c index 5801b7f674..96a79040c7 100644 --- a/lib/isc/hash.c +++ b/lib/isc/hash.c @@ -428,10 +428,14 @@ isc_hash_function(const void *data, size_t length, const unsigned char *bp; const unsigned char *be; + INSIST(data == NULL || length > 0); RUNTIME_CHECK(isc_once_do(&fnv_once, fnv_initialize) == ISC_R_SUCCESS); hval = previous_hashp != NULL ? *previous_hashp : fnv_offset_basis; + if (length == 0) + return (hval); + bp = (const unsigned char *) data; be = bp + length; @@ -490,10 +494,14 @@ isc_hash_function_reverse(const void *data, size_t length, const unsigned char *bp; const unsigned char *be; + INSIST(data == NULL || length > 0); RUNTIME_CHECK(isc_once_do(&fnv_once, fnv_initialize) == ISC_R_SUCCESS); hval = ISC_UNLIKELY(previous_hashp != NULL) ? *previous_hashp : fnv_offset_basis; + if (length == 0) + return (hval); + bp = (const unsigned char *) data; be = bp + length; diff --git a/lib/isc/tests/hash_test.c b/lib/isc/tests/hash_test.c index 0471b826d4..15c0cbc1ef 100644 --- a/lib/isc/tests/hash_test.c +++ b/lib/isc/tests/hash_test.c @@ -14,8 +14,6 @@ * PERFORMANCE OF THIS SOFTWARE. */ -/* $Id$ */ - /* ! \file */ #include @@ -25,6 +23,8 @@ #include #include +#include + #include #include #include @@ -1848,10 +1848,111 @@ ATF_TC_BODY(isc_crc64, tc) { } } +ATF_TC(isc_hash_function); +ATF_TC_HEAD(isc_hash_function, tc) { + atf_tc_set_md_var(tc, "descr", "Hash function test"); +} +ATF_TC_BODY(isc_hash_function, tc) { + unsigned int h1; + unsigned int h2; + + UNUSED(tc); + + /* Incremental hashing */ + + h1 = isc_hash_function(NULL, 0, ISC_TRUE, NULL); + h1 = isc_hash_function("This ", 5, ISC_TRUE, &h1); + h1 = isc_hash_function("is ", 3, ISC_TRUE, &h1); + h1 = isc_hash_function("a long test", 12, ISC_TRUE, &h1); + + h2 = isc_hash_function("This is a long test", 20, + ISC_TRUE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Immutability of hash function */ + h1 = isc_hash_function(NULL, 0, ISC_TRUE, NULL); + h2 = isc_hash_function(NULL, 0, ISC_TRUE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Hash function characteristics */ + h1 = isc_hash_function("Hello world", 12, ISC_TRUE, NULL); + h2 = isc_hash_function("Hello world", 12, ISC_TRUE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Case */ + h1 = isc_hash_function("Hello world", 12, ISC_FALSE, NULL); + h2 = isc_hash_function("heLLo WorLd", 12, ISC_FALSE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Unequal */ + h1 = isc_hash_function("Hello world", 12, ISC_TRUE, NULL); + h2 = isc_hash_function("heLLo WorLd", 12, ISC_TRUE, NULL); + + ATF_CHECK(h1 != h2); +} + + +ATF_TC(isc_hash_function_reverse); +ATF_TC_HEAD(isc_hash_function_reverse, tc) { + atf_tc_set_md_var(tc, "descr", "Reverse hash function test"); +} +ATF_TC_BODY(isc_hash_function_reverse, tc) { + unsigned int h1; + unsigned int h2; + + UNUSED(tc); + + /* Incremental hashing */ + + h1 = isc_hash_function_reverse(NULL, 0, ISC_TRUE, NULL); + h1 = isc_hash_function_reverse("\000", 1, ISC_TRUE, &h1); + h1 = isc_hash_function_reverse("\003org", 4, ISC_TRUE, &h1); + h1 = isc_hash_function_reverse("\007example", 8, ISC_TRUE, &h1); + + h2 = isc_hash_function_reverse("\007example\003org\000", 13, + ISC_TRUE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Immutability of hash function */ + h1 = isc_hash_function_reverse(NULL, 0, ISC_TRUE, NULL); + h2 = isc_hash_function_reverse(NULL, 0, ISC_TRUE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Hash function characteristics */ + h1 = isc_hash_function_reverse("Hello world", 12, ISC_TRUE, NULL); + h2 = isc_hash_function_reverse("Hello world", 12, ISC_TRUE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Case */ + h1 = isc_hash_function_reverse("Hello world", 12, ISC_FALSE, NULL); + h2 = isc_hash_function_reverse("heLLo WorLd", 12, ISC_FALSE, NULL); + + ATF_CHECK_EQ(h1, h2); + + /* Unequal */ + h1 = isc_hash_function_reverse("Hello world", 12, ISC_TRUE, NULL); + h2 = isc_hash_function_reverse("heLLo WorLd", 12, ISC_TRUE, NULL); + + ATF_CHECK(h1 != h2); +} + /* * Main */ ATF_TP_ADD_TCS(tp) { + /* + * Tests of hash functions, including isc_hash and the + * various cryptographic hashes. + */ + ATF_TP_ADD_TC(tp, isc_hash_function); + ATF_TP_ADD_TC(tp, isc_hash_function_reverse); ATF_TP_ADD_TC(tp, isc_hmacmd5); ATF_TP_ADD_TC(tp, isc_hmacsha1); ATF_TP_ADD_TC(tp, isc_hmacsha224); @@ -1865,6 +1966,6 @@ ATF_TP_ADD_TCS(tp) { ATF_TP_ADD_TC(tp, isc_sha384); ATF_TP_ADD_TC(tp, isc_sha512); ATF_TP_ADD_TC(tp, isc_crc64); + return (atf_no_error()); } -