From: George Thessalonikefs Date: Fri, 14 Jan 2022 12:55:34 +0000 (+0100) Subject: - Fix EDNS to upstream where the same option could be attached more than X-Git-Tag: release-1.15.0rc1~31^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=de1e91fc7f82f1880ad92848adb772f378ed3084;p=thirdparty%2Funbound.git - Fix EDNS to upstream where the same option could be attached more than once. - Add a region to serviced_query for allocations. --- diff --git a/services/outside_network.c b/services/outside_network.c index d2389dd2d..ca92707f3 100644 --- a/services/outside_network.c +++ b/services/outside_network.c @@ -1714,16 +1714,7 @@ static void serviced_node_del(rbnode_type* node, void* ATTR_UNUSED(arg)) { struct serviced_query* sq = (struct serviced_query*)node; - struct service_callback* p = sq->cblist, *np; - free(sq->qbuf); - free(sq->zone); - free(sq->tls_auth_name); - edns_opt_list_free(sq->opt_list); - while(p) { - np = p->next; - free(p); - p = np; - } + alloc_reg_release(sq->alloc, sq->region); free(sq); } @@ -2468,7 +2459,8 @@ serviced_create(struct outside_network* outnet, sldns_buffer* buff, int dnssec, int want_dnssec, int nocaps, int tcp_upstream, int ssl_upstream, char* tls_auth_name, struct sockaddr_storage* addr, socklen_t addrlen, uint8_t* zone, size_t zonelen, int qtype, struct edns_option* opt_list, - size_t pad_queries_block_size) + size_t pad_queries_block_size, struct alloc_cache* alloc, + struct regional* region) { struct serviced_query* sq = (struct serviced_query*)malloc(sizeof(*sq)); #ifdef UNBOUND_DEBUG @@ -2477,15 +2469,19 @@ serviced_create(struct outside_network* outnet, sldns_buffer* buff, int dnssec, if(!sq) return NULL; sq->node.key = sq; - sq->qbuf = memdup(sldns_buffer_begin(buff), sldns_buffer_limit(buff)); + sq->alloc = alloc; + sq->region = region; + sq->qbuf = regional_alloc_init(region, sldns_buffer_begin(buff), + sldns_buffer_limit(buff)); if(!sq->qbuf) { + alloc_reg_release(alloc, region); free(sq); return NULL; } sq->qbuflen = sldns_buffer_limit(buff); - sq->zone = memdup(zone, zonelen); + sq->zone = regional_alloc_init(region, zone, zonelen); if(!sq->zone) { - free(sq->qbuf); + alloc_reg_release(alloc, region); free(sq); return NULL; } @@ -2497,10 +2493,9 @@ serviced_create(struct outside_network* outnet, sldns_buffer* buff, int dnssec, sq->tcp_upstream = tcp_upstream; sq->ssl_upstream = ssl_upstream; if(tls_auth_name) { - sq->tls_auth_name = strdup(tls_auth_name); + sq->tls_auth_name = regional_strdup(region, tls_auth_name); if(!sq->tls_auth_name) { - free(sq->zone); - free(sq->qbuf); + alloc_reg_release(alloc, region); free(sq); return NULL; } @@ -2509,17 +2504,7 @@ serviced_create(struct outside_network* outnet, sldns_buffer* buff, int dnssec, } memcpy(&sq->addr, addr, addrlen); sq->addrlen = addrlen; - sq->opt_list = NULL; - if(opt_list) { - sq->opt_list = edns_opt_copy_alloc(opt_list); - if(!sq->opt_list) { - free(sq->tls_auth_name); - free(sq->zone); - free(sq->qbuf); - free(sq); - return NULL; - } - } + sq->opt_list = opt_list; sq->outnet = outnet; sq->cblist = NULL; sq->pending = NULL; @@ -2528,7 +2513,7 @@ serviced_create(struct outside_network* outnet, sldns_buffer* buff, int dnssec, sq->to_be_deleted = 0; sq->padding_block_size = pad_queries_block_size; #ifdef UNBOUND_DEBUG - ins = + ins = #else (void) #endif @@ -2898,7 +2883,8 @@ serviced_callbacks(struct serviced_query* sq, int error, struct comm_point* c, * use secondary buffer to store the query. * This is a data copy, but faster than packet to server */ backlen = sldns_buffer_limit(c->buffer); - backup_p = memdup(sldns_buffer_begin(c->buffer), backlen); + backup_p = regional_alloc_init(sq->region, + sldns_buffer_begin(c->buffer), backlen); if(!backup_p) { log_err("malloc failure in serviced query callbacks"); error = NETEVENT_CLOSED; @@ -2916,10 +2902,8 @@ serviced_callbacks(struct serviced_query* sq, int error, struct comm_point* c, } fptr_ok(fptr_whitelist_serviced_query(p->cb)); (void)(*p->cb)(c, p->cb_arg, error, rep); - free(p); } if(backup_p) { - free(backup_p); sq->outnet->svcd_overhead = 0; } verbose(VERB_ALGO, "svcd callbacks end"); @@ -3260,44 +3244,71 @@ outnet_serviced_query(struct outside_network* outnet, int nocaps, int tcp_upstream, int ssl_upstream, char* tls_auth_name, struct sockaddr_storage* addr, socklen_t addrlen, uint8_t* zone, size_t zonelen, struct module_qstate* qstate, - comm_point_callback_type* callback, void* callback_arg, sldns_buffer* buff, - struct module_env* env) + comm_point_callback_type* callback, void* callback_arg, + sldns_buffer* buff, struct module_env* env) { struct serviced_query* sq; struct service_callback* cb; struct edns_string_addr* client_string_addr; - - if(!inplace_cb_query_call(env, qinfo, flags, addr, addrlen, zone, zonelen, - qstate, qstate->region)) + struct regional* region; + struct edns_option* backed_up_opt_list = qstate->edns_opts_back_out; + struct edns_option* per_upstream_opt_list = NULL; + + /* If we have an already populated EDNS option list make a copy since + * we may now add upstream specific EDNS options. */ + /* Use a region that could be attached to a serviced_query, if it needs + * to be created. If an existing one is found then this region will be + * destroyed here. */ + region = alloc_reg_obtain(env->alloc); + if(!region) return NULL; + if(qstate->edns_opts_back_out) { + per_upstream_opt_list = edns_opt_copy_region( + qstate->edns_opts_back_out, region); + if(!per_upstream_opt_list) { + alloc_reg_release(env->alloc, region); return NULL; + } + qstate->edns_opts_back_out = per_upstream_opt_list; + } + + if(!inplace_cb_query_call(env, qinfo, flags, addr, addrlen, zone, + zonelen, qstate, region)) { + alloc_reg_release(env->alloc, region); + return NULL; + } + /* Restore the option list; we can explicitly use the copied one from + * now on. */ + qstate->edns_opts_back_out = backed_up_opt_list; if((client_string_addr = edns_string_addr_lookup( &env->edns_strings->client_strings, addr, addrlen))) { - edns_opt_list_append(&qstate->edns_opts_back_out, + edns_opt_list_append(&per_upstream_opt_list, env->edns_strings->client_string_opcode, client_string_addr->string_len, - client_string_addr->string, qstate->region); + client_string_addr->string, region); } serviced_gen_query(buff, qinfo->qname, qinfo->qname_len, qinfo->qtype, qinfo->qclass, flags); sq = lookup_serviced(outnet, buff, dnssec, addr, addrlen, - qstate->edns_opts_back_out); - /* duplicate entries are included in the callback list, because - * there is a counterpart registration by our caller that needs to - * be doubly-removed (with callbacks perhaps). */ - if(!(cb = (struct service_callback*)malloc(sizeof(*cb)))) - return NULL; + per_upstream_opt_list); if(!sq) { /* make new serviced query entry */ sq = serviced_create(outnet, buff, dnssec, want_dnssec, nocaps, tcp_upstream, ssl_upstream, tls_auth_name, addr, addrlen, zone, zonelen, (int)qinfo->qtype, - qstate->edns_opts_back_out, + per_upstream_opt_list, ( ssl_upstream && env->cfg->pad_queries - ? env->cfg->pad_queries_block_size : 0 )); + ? env->cfg->pad_queries_block_size : 0 ), + env->alloc, region); if(!sq) { - free(cb); + alloc_reg_release(env->alloc, region); + return NULL; + } + if(!(cb = (struct service_callback*)regional_alloc( + sq->region, sizeof(*cb)))) { + (void)rbtree_delete(outnet->serviced, sq); + serviced_node_del(&sq->node, NULL); return NULL; } /* perform first network action */ @@ -3305,17 +3316,25 @@ outnet_serviced_query(struct outside_network* outnet, if(!serviced_udp_send(sq, buff)) { (void)rbtree_delete(outnet->serviced, sq); serviced_node_del(&sq->node, NULL); - free(cb); return NULL; } } else { if(!serviced_tcp_send(sq, buff)) { (void)rbtree_delete(outnet->serviced, sq); serviced_node_del(&sq->node, NULL); - free(cb); return NULL; } } + } else { + /* We don't need this region anymore. */ + alloc_reg_release(env->alloc, region); + /* duplicate entries are included in the callback list, because + * there is a counterpart registration by our caller that needs + * to be doubly-removed (with callbacks perhaps). */ + if(!(cb = (struct service_callback*)regional_alloc( + sq->region, sizeof(*cb)))) { + return NULL; + } } /* add callback to list of callbacks */ cb->cb = callback; @@ -3334,7 +3353,6 @@ callback_list_remove(struct serviced_query* sq, void* cb_arg) if((*pp)->cb_arg == cb_arg) { struct service_callback* del = *pp; *pp = del->next; - free(del); return; } pp = &(*pp)->next; diff --git a/services/outside_network.h b/services/outside_network.h index d0d532e64..10d9d5ca6 100644 --- a/services/outside_network.h +++ b/services/outside_network.h @@ -43,7 +43,9 @@ #ifndef OUTSIDE_NETWORK_H #define OUTSIDE_NETWORK_H +#include "util/alloc.h" #include "util/rbtree.h" +#include "util/regional.h" #include "util/netevent.h" #include "dnstap/dnstap_config.h" struct pending; @@ -512,6 +514,11 @@ struct serviced_query { void* pending; /** block size with which to pad encrypted queries (default: 128) */ size_t padding_block_size; + /** region for this serviced query. Will be cleared when this + * serviced_query will be deleted */ + struct regional* region; + /** allocation service for the region */ + struct alloc_cache* alloc; }; /** diff --git a/testcode/fake_event.c b/testcode/fake_event.c index 6a8245d4a..e1b4443bd 100644 --- a/testcode/fake_event.c +++ b/testcode/fake_event.c @@ -1222,11 +1222,36 @@ struct serviced_query* outnet_serviced_query(struct outside_network* outnet, if(1) { struct edns_data edns; struct edns_string_addr* client_string_addr; + struct edns_option* backed_up_opt_list = + qstate->edns_opts_back_out; + struct edns_option* per_upstream_opt_list = NULL; + /* If we have an already populated EDNS option list make a copy + * since we may now add upstream specific EDNS options. */ + if(qstate->edns_opts_back_out) { + per_upstream_opt_list = edns_opt_copy_region( + qstate->edns_opts_back_out, qstate->region); + if(!per_upstream_opt_list) { + free(pend); + fatal_exit("out of memory"); + } + qstate->edns_opts_back_out = per_upstream_opt_list; + } if(!inplace_cb_query_call(env, qinfo, flags, addr, addrlen, zone, zonelen, qstate, qstate->region)) { free(pend); return NULL; } + /* Restore the option list; we can explicitly use the copied + * one from now on. */ + qstate->edns_opts_back_out = backed_up_opt_list; + if((client_string_addr = edns_string_addr_lookup( + &env->edns_strings->client_strings, + addr, addrlen))) { + edns_opt_list_append(&per_upstream_opt_list, + env->edns_strings->client_string_opcode, + client_string_addr->string_len, + client_string_addr->string, qstate->region); + } /* add edns */ edns.edns_present = 1; edns.ext_rcode = 0; @@ -1236,16 +1261,8 @@ struct serviced_query* outnet_serviced_query(struct outside_network* outnet, if(dnssec) edns.bits = EDNS_DO; edns.padding_block_size = 0; - if((client_string_addr = edns_string_addr_lookup( - &env->edns_strings->client_strings, - addr, addrlen))) { - edns_opt_list_append(&qstate->edns_opts_back_out, - env->edns_strings->client_string_opcode, - client_string_addr->string_len, - client_string_addr->string, qstate->region); - } edns.opt_list_in = NULL; - edns.opt_list_out = qstate->edns_opts_back_out; + edns.opt_list_out = per_upstream_opt_list; edns.opt_list_inplace_cb_out = NULL; attach_edns_record(pend->buffer, &edns); } diff --git a/testdata/edns_attached_once_per_upstream.rpl b/testdata/edns_attached_once_per_upstream.rpl new file mode 100644 index 000000000..19f1ba75d --- /dev/null +++ b/testdata/edns_attached_once_per_upstream.rpl @@ -0,0 +1,90 @@ +; config options +server: + edns-client-string: 10.0.0.0/24 "abc d" + outbound-msg-retry: 1 + +stub-zone: + name: "edns-string-abc." + stub-addr: 10.0.0.3 + stub-first: yes + +forward-zone: + name: "." + forward-addr: 10.0.0.1 + +CONFIG_END + +SCENARIO_BEGIN Test that upstream specific EDNS is attached once; uses string tag option + +RANGE_BEGIN 0 1000 + ADDRESS 10.0.0.3 +ENTRY_BEGIN +MATCH opcode qtype qname +ADJUST copy_id +REPLY QR SERVFAIL +SECTION QUESTION +edns-string-abc. IN A +ENTRY_END +RANGE_END + +RANGE_BEGIN 0 1000 + ADDRESS 10.0.0.1 +ENTRY_BEGIN +MATCH opcode qtype qname +ADJUST copy_id +REPLY QR NOERROR +SECTION QUESTION +edns-string-abc. IN A +SECTION ANSWER +edns-string-abc. IN A 10.20.30.40 +SECTION ADDITIONAL +ENTRY_END +RANGE_END + +STEP 10 QUERY +ENTRY_BEGIN +REPLY RD +SECTION QUESTION +edns-string-abc. IN A +ENTRY_END + +; This will receive SERVFAIL and the next address will be queried +STEP 20 CHECK_OUT_QUERY ADDRESS 10.0.0.3 +ENTRY_BEGIN +MATCH qname qtype opcode ednsdata +SECTION QUESTION +edns-string-abc. IN A +SECTION ADDITIONAL + HEX_EDNSDATA_BEGIN + fd e9 ; Opcode 65001 + 00 05 ; Length 5 + 61 62 63 20 64 ; "abc d" + HEX_EDNSDATA_END +ENTRY_END + +; This will receive the answer; makes sure that EDNS is attached once +STEP 22 CHECK_OUT_QUERY ADDRESS 10.0.0.1 +ENTRY_BEGIN +MATCH qname qtype opcode ednsdata +SECTION QUESTION +edns-string-abc. IN A +SECTION ADDITIONAL + HEX_EDNSDATA_BEGIN + fd e9 ; Opcode 65001 + 00 05 ; Length 5 + 61 62 63 20 64 ; "abc d" + HEX_EDNSDATA_END +ENTRY_END + + +STEP 30 CHECK_ANSWER +ENTRY_BEGIN +MATCH all +REPLY QR RD RA NOERROR +SECTION QUESTION +edns-string-abc. IN A +SECTION ANSWER +edns-string-abc. IN A 10.20.30.40 +ENTRY_END + +SCENARIO_END