From: Yuri Schaeffer Date: Tue, 17 Sep 2013 09:28:36 +0000 (+0000) Subject: Low hanging fruit review comments. X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=22ddd25651641478a42274fa920a03cc1c586173;p=thirdparty%2Funbound.git Low hanging fruit review comments. git-svn-id: file:///svn/unbound/branches/edns-subnet@2956 be551aaa-1e26-0410-a405-d3ace91eadb9 --- diff --git a/edns-subnet/addrtree.c b/edns-subnet/addrtree.c index b638b2080..433695c29 100644 --- a/edns-subnet/addrtree.c +++ b/edns-subnet/addrtree.c @@ -7,11 +7,8 @@ /** \file * see addrtree.h */ - -#include -#include -#include -#include +#include "config.h" +#include "util/log.h" #include "util/data/msgreply.h" #include "util/module.h" #include "addrtree.h" @@ -64,7 +61,7 @@ node_create(struct reply_info* elem, addrlen_t scope) struct addrtree* addrtree_create(addrlen_t max_depth, struct module_env* env) { struct addrtree* tree; - assert(env != NULL); + log_assert(env != NULL); tree = (struct addrtree*)malloc( sizeof(*tree) ); if(!tree) return NULL; @@ -113,7 +110,6 @@ size_t addrtree_size(const struct addrtree* tree) void addrtree_clean_node(const struct addrtree* tree, struct addrnode* node) { if (node->elem) { - //~ reply_info_parsedelete(node->elem, tree->env->alloc); reply_info_delete(node->elem, NULL); node->elem = NULL; } @@ -133,8 +129,8 @@ freenode_recursive(struct addrtree* tree, struct addrnode* node) for (i = 0; i < 2; i++) { edge = node->edge[i]; if (edge) { - assert(edge->node != NULL); - assert(edge->str != NULL); + log_assert(edge->node != NULL); + log_assert(edge->str != NULL); freenode_recursive(tree, edge->node); free(edge->str); } @@ -156,7 +152,7 @@ void addrtree_delete(struct addrtree* tree) /** Get N'th bit from address */ int getbit(const addrkey_t* addr, addrlen_t addrlen, addrlen_t n) { - assert(addrlen > n); + log_assert(addrlen > n); return (int)(addr[n/KEYWIDTH]>>((KEYWIDTH-1)-(n%KEYWIDTH))) & 1; } @@ -185,7 +181,7 @@ bits_common(const addrkey_t* s1, addrlen_t l1, { addrlen_t len, i; len = (l1 > l2) ? l2 : l1; - assert(skip < len); + log_assert(skip < len); for (i = skip; i < len; i++) { if (cmpbit(s1, s2, i)) return i; } @@ -214,20 +210,20 @@ addrtree_insert(struct addrtree* tree, const addrkey_t* addr, { struct addrnode* newnode, *node; struct addredge* edge, *newedge; - size_t index; + uint8_t index; addrlen_t common, depth; node = tree->root; - assert(node != NULL); + log_assert(node != NULL); - /* Protect our cache against to much fine-grained data */ + /* Protect our cache against too much fine-grained data */ if (tree->max_depth < scope) scope = tree->max_depth; /* Server answer was less specific than question */ if (scope < sourcemask) sourcemask = scope; depth = 0; while (1) { - assert(depth <= sourcemask); + log_assert(depth <= sourcemask); /* Case 1: update existing node */ if (depth == sourcemask) { /* update this node's scope and data */ @@ -238,12 +234,14 @@ addrtree_insert(struct addrtree* tree, const addrkey_t* addr, node->scope = scope; return; } - index = (size_t)getbit(addr, sourcemask, depth); + index = (uint8_t)getbit(addr, sourcemask, depth); edge = node->edge[index]; /* Case 2: New leafnode */ if (!edge) { newnode = node_create(elem, scope); node->edge[index] = edge_create(newnode, addr, sourcemask); + if (!node->edge[index]) + free(newnode); return; } /* Case 3: Traverse edge */ @@ -258,10 +256,14 @@ addrtree_insert(struct addrtree* tree, const addrkey_t* addr, continue; } /* Case 4: split. */ - newnode = node_create(NULL, 0); - newedge = edge_create(newnode, addr, common); + if (!(newnode = node_create(NULL, 0))) + return; + if (!(newedge = edge_create(newnode, addr, common))) { + free(newnode); + return; + } node->edge[index] = newedge; - index = (size_t)getbit(edge->str, edge->len, common); + index = (uint8_t)getbit(edge->str, edge->len, common); newnode->edge[index] = edge; if (common == sourcemask) { @@ -286,13 +288,13 @@ addrtree_find(const struct addrtree* tree, const addrkey_t* addr, struct addredge* edge; addrlen_t depth = 0; - assert(node != NULL); + log_assert(node != NULL); while (1) { /* Current node more specific then question. */ - assert(depth <= sourcemask); + log_assert(depth <= sourcemask); /* does this node have data? if yes, see if we have a match */ if (node->elem) { - assert(node->scope >= depth); /* saved at wrong depth */ + log_assert(node->scope >= depth); /* saved at wrong depth */ if (depth == node->scope || (node->scope > sourcemask && depth == sourcemask)) { /* Authority indicates it does not have a more precise @@ -311,7 +313,7 @@ addrtree_find(const struct addrtree* tree, const addrkey_t* addr, return NULL; if (!issub(edge->str, edge->len, addr, sourcemask, depth)) return NULL; - assert(depth < edge->len); + log_assert(depth < edge->len); depth = edge->len; node = edge->node; } diff --git a/edns-subnet/edns-subnet.h b/edns-subnet/edns-subnet.h index 1c2bd2043..f27a2eacf 100644 --- a/edns-subnet/edns-subnet.h +++ b/edns-subnet/edns-subnet.h @@ -16,7 +16,7 @@ #define EDNSSUBNET_ADDRFAM_IP4 1 #define EDNSSUBNET_ADDRFAM_IP6 2 -/** Opcode for edns subnet option, is TBD. */ +/** Opcode for edns subnet option */ extern uint16_t EDNSSUBNET_OPCODE; /** Maximum number of bits we are willing to expose */ extern uint8_t EDNSSUBNET_MAX_SUBNET_IP4; diff --git a/edns-subnet/subnetmod.c b/edns-subnet/subnetmod.c index 22847cedc..83d6d7d97 100644 --- a/edns-subnet/subnetmod.c +++ b/edns-subnet/subnetmod.c @@ -108,86 +108,78 @@ void cp_edns_bad_response(struct edns_data* target, struct edns_data* source) target->subnet_validdata = 1; } + +/* select tree from cache entry based on edns data. + * If for address family not present it will create a new one. + * NULL on failure to create. */ +static struct addrtree* +get_tree(struct subnet_msg_cache_data* data, struct edns_data* edns, + struct module_env* env) +{ + struct addrtree* tree; + if (edns->subnet_addr_fam == EDNSSUBNET_ADDRFAM_IP4) { + if (!data->tree4) + data->tree4 = addrtree_create(EDNSSUBNET_MAX_SUBNET_IP4, env); + tree = data->tree4; + } else { + if (!data->tree6) + data->tree6 = addrtree_create(EDNSSUBNET_MAX_SUBNET_IP6, env); + tree = data->tree6; + } + return tree; +} + void update_cache(struct module_qstate* qstate, int id) { - hashvalue_t h; - struct lruhash_entry* lru_entry; struct msgreply_entry* mrep_entry; struct addrtree* tree; struct reply_info *rep; struct query_info qinf; - struct subnet_msg_cache_data* data; - struct module_env* env = qstate->env; - struct subnet_env* sne = (struct subnet_env*)env->modinfo[id]; - struct subnet_qstate* iq = (struct subnet_qstate*)qstate->minfo[id]; - struct slabhash* subnet_msg_cache = sne->subnet_msg_cache; - struct query_info* qinfo = &qstate->qinfo; + struct slabhash* subnet_msg_cache = + ((struct subnet_env*)qstate->env->modinfo[id])->subnet_msg_cache; struct edns_data* edns = &qstate->edns_client_in; - int acquired_lock = 0; /** We already calculated hash upon lookup */ - h = iq ? iq->qinfo_hash : query_info_hash(qinfo); - + hashvalue_t h = qstate->minfo[id] ? + ((struct subnet_qstate*)qstate->minfo[id])->qinfo_hash : + query_info_hash(&qstate->qinfo); /** Step 1, general qinfo lookup */ - lru_entry = slabhash_lookup(subnet_msg_cache, h, qinfo, 1); + struct lruhash_entry* lru_entry = slabhash_lookup(subnet_msg_cache, h, &qstate->qinfo, 1); + int acquired_lock = (lru_entry != NULL); if (!lru_entry) { - data = (struct subnet_msg_cache_data*) malloc( - sizeof(struct subnet_msg_cache_data) ); - if (!data) { - log_err("malloc failed"); - return; - } - data->tree4 = NULL; - data->tree6 = NULL; - - qinf = *qinfo; - qinf.qname = memdup(qinfo->qname, qinfo->qname_len); + qinf = qstate->qinfo; + qinf.qname = memdup(qstate->qinfo.qname, qstate->qinfo.qname_len); if(!qinf.qname) { log_err("malloc failed"); - free(data); return; } mrep_entry = query_info_entrysetup(&qinf, NULL, h); + free(qinf.qname); /* if qname 'consumed', it is set to NULL */ if (!mrep_entry) { log_err("query_info_entrysetup failed"); - free(data); return; } - free(qinf.qname); /* if qname 'consumed', it is set to NULL */ lru_entry = &mrep_entry->entry; - lru_entry->data = data; - slabhash_insert(subnet_msg_cache, h, lru_entry, data, NULL); - } else { - data = lru_entry->data; - acquired_lock = 1; + lru_entry->data = calloc(1, sizeof(struct subnet_msg_cache_data)); + if (!lru_entry->data) { + log_err("malloc failed"); + return; + } + slabhash_insert(subnet_msg_cache, h, lru_entry, lru_entry->data, NULL); } /** Step 2, find the correct tree */ - if (edns->subnet_addr_fam == EDNSSUBNET_ADDRFAM_IP4) { - if (!data->tree4) - data->tree4 = addrtree_create(EDNSSUBNET_MAX_SUBNET_IP4, env); - tree = data->tree4; - } else { - if (!data->tree6) - data->tree6 = addrtree_create(EDNSSUBNET_MAX_SUBNET_IP6, env); - tree = data->tree6; - } - assert(tree != NULL); /* TODO remove me */ - if (!tree) { + if (!(tree = get_tree(lru_entry->data, edns, qstate->env))) { + if (acquired_lock) lock_rw_unlock(&lru_entry->lock); log_err("Subnet cache insertion failed"); - if (acquired_lock) - lock_rw_unlock(&lru_entry->lock); return; } - - rep = reply_info_copy(qstate->return_msg->rep, env->alloc, NULL); - /* fixup flags to be sensible for a reply based on the cache */ - rep->flags |= (BIT_RA | BIT_QR); - rep->flags &= ~(BIT_AA | BIT_CD); - - addrtree_insert(tree, (addrkey_t*)edns->subnet_addr, edns->subnet_source_mask, + rep = reply_info_copy(qstate->return_msg->rep, qstate->env->alloc, NULL); + rep->flags |= (BIT_RA | BIT_QR); /* fix flags to be sensible for */ + rep->flags &= ~(BIT_AA | BIT_CD);/* a reply based on the cache */ + addrtree_insert(tree, (addrkey_t*)edns->subnet_addr, + edns->subnet_source_mask, qstate->edns_server_in.subnet_scope_mask, rep); - if (acquired_lock) - lock_rw_unlock(&lru_entry->lock); + if (acquired_lock) lock_rw_unlock(&lru_entry->lock); } @@ -223,7 +215,6 @@ int lookup_and_reply(struct module_qstate* qstate, int id) } rep = node->elem; if(rep->ttl < *env->now) { /** msg expired, remove from node */ - node->elem = NULL; addrtree_clean_node(tree, node); lock_rw_unlock(&e->lock); return 0; @@ -246,13 +237,25 @@ int lookup_and_reply(struct module_qstate* qstate, int id) return 1; } +/* Test first bits of addresses for equality. Caller is responsible + * for making sure that both a and b are at least net/8 octets long. + * a,b: addresses + * net: Number of bits to test + * return: 1 if equal, 0 otherwise*/ +static int +common_prefix(uint8_t *a, uint8_t *b, uint8_t net) +{ + int n = net / 8; + return !memcmp(a, b, n) && ((net % 8) == 0 || a[n] == b[n]); +} + enum module_ext_state eval_response(struct module_qstate* qstate, int id) { size_t sn_octs; - struct edns_data *c_in = &qstate->edns_client_in; - struct edns_data *c_out = &qstate->edns_client_out; - struct edns_data *s_in = &qstate->edns_server_in; - struct edns_data *s_out = &qstate->edns_server_out; + struct edns_data *c_in = &qstate->edns_client_in; /* rcvd from client */ + struct edns_data *c_out = &qstate->edns_client_out;/* will send to client */ + struct edns_data *s_in = &qstate->edns_server_in; /* rcvd from auth */ + struct edns_data *s_out = &qstate->edns_server_out;/* sent to auth */ /** We have not asked for subnet data */ if (!s_out->subnet_sent) { @@ -281,12 +284,11 @@ enum module_ext_state eval_response(struct module_qstate* qstate, int id) * anywhere. */ /** can we accept response? */ - sn_octs = s_out->subnet_source_mask / 8; assert(sn_octs <= INET6_SIZE); /** Enforced by msgparse */ if(s_out->subnet_addr_fam != s_in->subnet_addr_fam || s_out->subnet_source_mask != s_in->subnet_source_mask || - memcmp(s_out->subnet_addr, s_in->subnet_addr, sn_octs) != 0 || - s_out->subnet_addr[sn_octs] != s_in->subnet_addr[sn_octs]) + !common_prefix(s_out->subnet_addr, s_in->subnet_addr, + s_out->subnet_source_mask)) { /** we can not accept, restart query without option */ verbose(VERB_QUERY, "subnet: forged data"); @@ -327,7 +329,7 @@ void subnetmod_operate(struct module_qstate* qstate, enum module_ev event, } if (lookup_and_reply(qstate, id)) { - verbose(VERB_QUERY, "subnet: answerd from cache"); + verbose(VERB_QUERY, "subnet: answered from cache"); qstate->ext_state[id] = module_finished; return; } diff --git a/edns-subnet/subnetmod.h b/edns-subnet/subnetmod.h index 1b43b11df..48b310188 100644 --- a/edns-subnet/subnetmod.h +++ b/edns-subnet/subnetmod.h @@ -22,7 +22,9 @@ * Global state for the subnet module. */ struct subnet_env { - /** shared message cache */ + /** shared message cache + * key: struct query_info* + * data: struct subnet_msg_cache_data* */ struct slabhash* subnet_msg_cache; }; diff --git a/services/mesh.c b/services/mesh.c index d1b6a53d2..96e905dbe 100644 --- a/services/mesh.c +++ b/services/mesh.c @@ -144,7 +144,50 @@ int subnet_compare(struct edns_data *a, struct edns_data *b) } return memcmp(b->subnet_addr, a->subnet_addr, INET6_SIZE); } -#endif + +/* Populate edns structure with subnet data either from subnet option + * or IP layer. + * edns: allocated edns structure to populate + * s: mesh state of query + */ +static void +populate_edns_subnet(struct edns_data* edns, struct mesh_state* s) +{ + edns->subnet_downstream = edns->subnet_validdata; + if(!edns->subnet_validdata) { + struct sockaddr_storage *ss; + void* sinaddr; + edns->subnet_validdata = 0; + /* Construct subnet option from original query */ + ss = &s->reply_list->query_reply.addr; + if(((struct sockaddr_in*)ss)->sin_family == AF_INET) { + edns->subnet_source_mask = EDNSSUBNET_MAX_SUBNET_IP4; + edns->subnet_addr_fam = EDNSSUBNET_ADDRFAM_IP4; + sinaddr = &((struct sockaddr_in*)ss)->sin_addr; + if (!copy_clear( + edns->subnet_addr, INET6_SIZE, (uint8_t *)sinaddr, + INET_SIZE, EDNSSUBNET_MAX_SUBNET_IP4)) { + edns->subnet_validdata = 1; + } + } +#ifdef INET6 + else { + edns->subnet_source_mask = EDNSSUBNET_MAX_SUBNET_IP6; + edns->subnet_addr_fam = EDNSSUBNET_ADDRFAM_IP6; + sinaddr = &((struct sockaddr_in6*)ss)->sin6_addr; + if (!copy_clear( + edns->subnet_addr, INET6_SIZE, (uint8_t *)sinaddr, + INET6_SIZE, EDNSSUBNET_MAX_SUBNET_IP6)) { + edns->subnet_validdata = 1; + } + } +#else + /* We don't know how to handle ip6, just pass */ +#endif /* INET6 */ + } + memcpy(&s->s.edns_client_in, edns, sizeof(struct edns_data)); +} +#endif /* CLIENT_SUBNET */ int mesh_state_compare(const void* ap, const void* bp) @@ -379,44 +422,9 @@ void mesh_new_client(struct mesh_area* mesh, struct query_info* qinfo, return; } #ifdef CLIENT_SUBNET - if(edns->subnet_validdata) { - edns->subnet_downstream = 1; - } else { - struct sockaddr_storage *ss; - void* sinaddr; - edns->subnet_validdata = 0; - edns->subnet_downstream = 0; - /* Construct subnet option from original query */ - ss = &s->reply_list->query_reply.addr; - if(((struct sockaddr_in*)ss)->sin_family == AF_INET) { - edns->subnet_source_mask = EDNSSUBNET_MAX_SUBNET_IP4; - edns->subnet_addr_fam = EDNSSUBNET_ADDRFAM_IP4; - sinaddr = &((struct sockaddr_in*)ss)->sin_addr; - if (!copy_clear( - edns->subnet_addr, INET6_SIZE, (uint8_t *)sinaddr, - INET_SIZE, EDNSSUBNET_MAX_SUBNET_IP4)) { - edns->subnet_validdata = 1; - } - } -#ifdef INET6 - else { - edns->subnet_source_mask = EDNSSUBNET_MAX_SUBNET_IP6; - edns->subnet_addr_fam = EDNSSUBNET_ADDRFAM_IP6; - sinaddr = &((struct sockaddr_in6*)ss)->sin6_addr; - if (!copy_clear( - edns->subnet_addr, INET6_SIZE, (uint8_t *)sinaddr, - INET6_SIZE, EDNSSUBNET_MAX_SUBNET_IP6)) { - edns->subnet_validdata = 1; - } - } -#else - else { - /* We don't know how to handle ip6 , just pass*/ - } -#endif /* INET6 */ - } - memcpy(&s->s.edns_client_in, edns, sizeof(struct edns_data)); + populate_edns_subnet(edns, s); #endif /* CLIENT_SUBNET */ + /* update statistics */ if(was_detached) { log_assert(mesh->num_detached_states > 0); diff --git a/testcode/ldns-testpkts.c b/testcode/ldns-testpkts.c index a6c60b10a..6564223ae 100644 --- a/testcode/ldns-testpkts.c +++ b/testcode/ldns-testpkts.c @@ -741,7 +741,7 @@ match_ednsdata(ldns_pkt* q, struct reply_packet* p) else if (!p_set || !q_set) return 0; else - return qdlen == pdlen && 0 == memcmp(qd, pd, qdlen); + return qdlen == pdlen && memcmp(qd, pd, qdlen) == 0; } /* finds entry in list, or returns NULL */ diff --git a/util/configparser.c b/util/configparser.c index c4d9eee9e..d0ec5107f 100644 --- a/util/configparser.c +++ b/util/configparser.c @@ -2223,10 +2223,10 @@ yyreduce: else if(atoi((yyvsp[(2) - (2)].str)) > 65535 || atoi((yyvsp[(2) - (2)].str)) < 0) yyerror("option code must be in interval [0, 65535]"); else cfg_parser->cfg->client_subnet_opcode = atoi((yyvsp[(2) - (2)].str)); - free((yyvsp[(2) - (2)].str)); #else OUTYY(("P(Compiled without edns subnet option, ignoring)\n")); #endif + free((yyvsp[(2) - (2)].str)); } break; @@ -2243,10 +2243,10 @@ yyreduce: else if (atoi((yyvsp[(2) - (2)].str)) < 0) cfg_parser->cfg->max_client_subnet_ipv4 = 0; else cfg_parser->cfg->max_client_subnet_ipv4 = atoi((yyvsp[(2) - (2)].str)); - free((yyvsp[(2) - (2)].str)); #else OUTYY(("P(Compiled without edns subnet option, ignoring)\n")); #endif + free((yyvsp[(2) - (2)].str)); } break; @@ -2263,10 +2263,10 @@ yyreduce: else if (atoi((yyvsp[(2) - (2)].str)) < 0) cfg_parser->cfg->max_client_subnet_ipv6 = 0; else cfg_parser->cfg->max_client_subnet_ipv6 = atoi((yyvsp[(2) - (2)].str)); - free((yyvsp[(2) - (2)].str)); #else OUTYY(("P(Compiled without edns subnet option, ignoring)\n")); #endif + free((yyvsp[(2) - (2)].str)); } break; diff --git a/util/configparser.y b/util/configparser.y index 7c8178b5d..484ee42be 100644 --- a/util/configparser.y +++ b/util/configparser.y @@ -277,10 +277,10 @@ server_client_subnet_opcode: VAR_CLIENT_SUBNET_OPCODE STRING_ARG else if(atoi($2) > 65535 || atoi($2) < 0) yyerror("option code must be in interval [0, 65535]"); else cfg_parser->cfg->client_subnet_opcode = atoi($2); - free($2); #else OUTYY(("P(Compiled without edns subnet option, ignoring)\n")); #endif + free($2); } ; server_max_client_subnet_ipv4: VAR_MAX_CLIENT_SUBNET_IPV4 STRING_ARG @@ -294,10 +294,10 @@ server_max_client_subnet_ipv4: VAR_MAX_CLIENT_SUBNET_IPV4 STRING_ARG else if (atoi($2) < 0) cfg_parser->cfg->max_client_subnet_ipv4 = 0; else cfg_parser->cfg->max_client_subnet_ipv4 = atoi($2); - free($2); #else OUTYY(("P(Compiled without edns subnet option, ignoring)\n")); #endif + free($2); } ; server_max_client_subnet_ipv6: VAR_MAX_CLIENT_SUBNET_IPV6 STRING_ARG @@ -311,10 +311,10 @@ server_max_client_subnet_ipv6: VAR_MAX_CLIENT_SUBNET_IPV6 STRING_ARG else if (atoi($2) < 0) cfg_parser->cfg->max_client_subnet_ipv6 = 0; else cfg_parser->cfg->max_client_subnet_ipv6 = atoi($2); - free($2); #else OUTYY(("P(Compiled without edns subnet option, ignoring)\n")); #endif + free($2); } ; server_interface: VAR_INTERFACE STRING_ARG diff --git a/util/data/msgparse.c b/util/data/msgparse.c index 73a43e654..7a91912f0 100644 --- a/util/data/msgparse.c +++ b/util/data/msgparse.c @@ -977,12 +977,12 @@ parse_ednsdata(uint8_t* data, struct edns_data* edns) opt_opc = ldns_read_uint16(&data[opt_start]); opt_len = ldns_read_uint16(&data[2 + opt_start]); /* Option does not fit in remaining data */ - if(opt_start + 4 + opt_len > edns_datalen) break; + if(opt_start + 4 + (int)opt_len > edns_datalen) break; if(opt_opc == EDNSSUBNET_OPCODE) { parse_subnet_option(data + opt_start + 4, edns, opt_len); } else { /* Unknown opcode */ - verbose(VERB_QUERY, "unknown EDNS option %x", opt_opc); + verbose(VERB_QUERY, "unknown EDNS option 0x%x", opt_opc); } opt_start += opt_len + 4; @@ -1045,8 +1045,9 @@ parse_extract_edns(struct msg_parse* msg, struct edns_data* edns) edns->bits = ldns_read_uint16(&found->rr_last->ttl_data[2]); #ifdef CLIENT_SUBNET parse_ednsdata(found->rr_last->ttl_data + 4, edns); -#endif +#else /* ignore rdata and rrsigs */ +#endif return 0; }