From: George Thessalonikefs Date: Mon, 17 Jul 2023 15:26:31 +0000 (+0200) Subject: Review for #759: X-Git-Tag: release-1.18.0rc1~24^2^2~6 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f5a2a58ce3fce117b9e478f0032ea8d81ba05344;p=thirdparty%2Funbound.git Review for #759: - Fix SEGFAULT in load_cache control command. - Change reason_bogus_str to an explicit NULL-terminated string. - Fix potential memory leak when discarding a message for referrals and 0 TTL answers. - Fix reason_bogus initialization in localzone answers. - reply_info creation in validator is always regional. --- diff --git a/daemon/cachedump.c b/daemon/cachedump.c index c915c5b0b..8d0466c49 100644 --- a/daemon/cachedump.c +++ b/daemon/cachedump.c @@ -652,6 +652,7 @@ load_msg(RES* ssl, sldns_buffer* buf, struct worker* worker) log_warn("error cannot parse numbers: %s", s); return 0; } + memset(&rep, 0, sizeof(rep)); rep.flags = (uint16_t)flags; rep.qdcount = (uint16_t)qdcount; rep.ttl = (time_t)ttl; diff --git a/daemon/worker.c b/daemon/worker.c index 505616b39..529b22ae4 100644 --- a/daemon/worker.c +++ b/daemon/worker.c @@ -507,8 +507,8 @@ answer_norec_from_cache(struct worker* worker, struct query_info* qinfo, msg->rep, LDNS_RCODE_SERVFAIL, edns, repinfo, worker->scratchpad, worker->env.now_tv)) return 0; - /* Attached the cached EDE (RFC8914) */ - if (worker->env.cfg->ede) { + /* Attach the cached EDE (RFC8914) */ + if(worker->env.cfg->ede) { edns_opt_list_append_ede(&edns->opt_list_out, worker->scratchpad, msg->rep->reason_bogus, msg->rep->reason_bogus_str); @@ -693,8 +693,8 @@ answer_from_cache(struct worker* worker, struct query_info* qinfo, LDNS_RCODE_SERVFAIL, edns, repinfo, worker->scratchpad, worker->env.now_tv)) goto bail_out; - /* Attached the cached EDE (RFC8914) */ - if (worker->env.cfg->ede) { + /* Attach the cached EDE (RFC8914) */ + if(worker->env.cfg->ede) { edns_opt_list_append_ede(&edns->opt_list_out, worker->scratchpad, rep->reason_bogus, rep->reason_bogus_str); @@ -1668,7 +1668,7 @@ worker_handle_request(struct comm_point* c, void* arg, int error, * ACLs allow the snooping. */ if(!(LDNS_RD_WIRE(sldns_buffer_begin(c->buffer))) && acl != acl_allow_snoop ) { - if (worker->env.cfg->ede) { + if(worker->env.cfg->ede) { EDNS_OPT_LIST_APPEND_EDE(&edns.opt_list_out, worker->scratchpad, LDNS_EDE_NOT_AUTHORITATIVE, ""); } diff --git a/iterator/iterator.c b/iterator/iterator.c index 8fe8a0c99..d1f4bc955 100644 --- a/iterator/iterator.c +++ b/iterator/iterator.c @@ -358,7 +358,6 @@ error_response_cache(struct module_qstate* qstate, int id, int rcode) err.serve_expired_ttl = NORR_TTL; /* do not waste time trying to validate this servfail */ err.security = sec_status_indeterminate; - err.reason_bogus_str = NULL; verbose(VERB_ALGO, "store error response in message cache"); iter_dns_store(qstate->env, &qstate->qinfo, &err, 0, 0, 0, NULL, qstate->query_flags, qstate->qstarttime); @@ -3827,8 +3826,8 @@ processFinished(struct module_qstate* qstate, struct iter_qstate* iq, /* make sure QR flag is on */ iq->response->rep->flags |= BIT_QR; - /* explicitly set the EDE string size to 0 */ - iq->response->rep->reason_bogus_str_size = 0; + /* explicitly set the EDE string to NULL */ + iq->response->rep->reason_bogus_str = NULL; /* we have finished processing this query */ qstate->ext_state[id] = module_finished; diff --git a/services/cache/dns.c b/services/cache/dns.c index 225a68c61..a3d029231 100644 --- a/services/cache/dns.c +++ b/services/cache/dns.c @@ -157,7 +157,7 @@ dns_cache_store_msg(struct module_env* env, struct query_info* qinfo, /* we do not store the message, but we did store the RRs, * which could be useful for delegation information */ verbose(VERB_ALGO, "TTL 0: dropped msg from cache"); - free(rep); + reply_info_delete(rep, NULL); /* if the message is in the cache, remove that msg, * so that the TTL 0 response can be returned for future * responses (i.e. don't get answered from @@ -1073,7 +1073,7 @@ dns_cache_store(struct module_env* env, struct query_info* msgqinf, ((ntohs(ref.key->rk.type)==LDNS_RR_TYPE_NS && !pside) ? qstarttime:*env->now + leeway)); } - free(rep); + reply_info_delete(rep, NULL); return 1; } else { /* store msg, and rrsets */ diff --git a/services/localzone.c b/services/localzone.c index 48fa730b5..44da22d78 100644 --- a/services/localzone.c +++ b/services/localzone.c @@ -1308,6 +1308,7 @@ local_encode(struct query_info* qinfo, struct module_env* env, else rep.ns_numrrsets = 1; rep.rrset_count = 1; rep.rrsets = &rrset; + rep.reason_bogus = LDNS_EDE_NONE; udpsize = edns->udp_size; edns->edns_version = EDNS_ADVERTISED_VERSION; edns->udp_size = EDNS_ADVERTISED_SIZE; diff --git a/util/data/msgreply.c b/util/data/msgreply.c index b1884d023..792387444 100644 --- a/util/data/msgreply.c +++ b/util/data/msgreply.c @@ -117,16 +117,9 @@ construct_reply_info_base(struct regional* region, uint16_t flags, size_t qd, rep->ar_numrrsets = ar; rep->rrset_count = total; rep->security = sec; - /* verify that we set the EDE to none by setting it explicitly */ - if (reason_bogus != LDNS_EDE_NONE) { - rep->reason_bogus = reason_bogus; - } else { - rep->reason_bogus = LDNS_EDE_NONE; - } + rep->reason_bogus = reason_bogus; /* this is only allocated and used for caching on copy */ rep->reason_bogus_str = NULL; - rep->reason_bogus_str_size = 0; - rep->authoritative = 0; /* array starts after the refs */ if(region) @@ -589,9 +582,9 @@ reply_info_parsedelete(struct reply_info* rep, struct alloc_cache* alloc) for(i=0; irrset_count; i++) { ub_packed_rrset_parsedelete(rep->rrsets[i], alloc); } - - if (rep->reason_bogus_str_size) { + if(rep->reason_bogus_str) { free(rep->reason_bogus_str); + rep->reason_bogus_str = NULL; } free(rep); } @@ -674,8 +667,9 @@ void reply_info_delete(void* d, void* ATTR_UNUSED(arg)) { struct reply_info* r = (struct reply_info*)d; - if (r->reason_bogus_str_size) { + if(r->reason_bogus_str) { free(r->reason_bogus_str); + r->reason_bogus_str = NULL; } free(r); } @@ -753,35 +747,34 @@ repinfo_copy_rrsets(struct reply_info* dest, struct reply_info* from, return 1; } -struct reply_info* -reply_info_copy(struct reply_info* rep, struct alloc_cache* alloc, +struct reply_info* +reply_info_copy(struct reply_info* rep, struct alloc_cache* alloc, struct regional* region) { struct reply_info* cp; - cp = construct_reply_info_base(region, rep->flags, rep->qdcount, - rep->ttl, rep->prefetch_ttl, rep->serve_expired_ttl, + cp = construct_reply_info_base(region, rep->flags, rep->qdcount, + rep->ttl, rep->prefetch_ttl, rep->serve_expired_ttl, rep->an_numrrsets, rep->ns_numrrsets, rep->ar_numrrsets, rep->rrset_count, rep->security, rep->reason_bogus); if(!cp) return NULL; - if (rep->reason_bogus_str_size > 0 && rep->reason_bogus_str) { - if (region) { + if(rep->reason_bogus_str && *rep->reason_bogus_str != 0) { + if(region) { cp->reason_bogus_str = (char*)regional_alloc(region, - sizeof(char) * (rep->reason_bogus_str_size + 1)); - } - else { - cp->reason_bogus_str = malloc(sizeof(char) * (rep->reason_bogus_str_size + 1)); + sizeof(char) + * (strlen(rep->reason_bogus_str)+1)); + } else { + cp->reason_bogus_str = malloc(sizeof(char) + * (strlen(rep->reason_bogus_str)+1)); } - - if (!(cp->reason_bogus_str)) { + if(!cp->reason_bogus_str) { if(!region) reply_info_parsedelete(cp, alloc); return NULL; } memcpy(cp->reason_bogus_str, rep->reason_bogus_str, - rep->reason_bogus_str_size+1); - cp->reason_bogus_str_size = rep->reason_bogus_str_size; + strlen(rep->reason_bogus_str)+1); } /* allocate ub_key structures special or not */ diff --git a/util/data/msgreply.h b/util/data/msgreply.h index 9ee402f84..1339fd9cc 100644 --- a/util/data/msgreply.h +++ b/util/data/msgreply.h @@ -170,20 +170,17 @@ struct reply_info { /** * EDE (rfc8914) code with reason for DNSSEC bogus status. + * Used for caching the EDE. */ sldns_ede_code reason_bogus; /** - * EDE (rfc8914) text string with human-readable reason for DNSSEC - * bogus status. Used for caching the EDE. + * EDE (rfc8914) NULL-terminated string with human-readable reason + * for DNSSEC bogus status. + * Used for caching the EDE. */ char* reason_bogus_str; - /** - * EDE (rfc8914) text string size. - */ - size_t reason_bogus_str_size; - /** * Number of RRsets in each section. * The answer section. Add up the RRs in every RRset to calculate @@ -251,15 +248,15 @@ struct msgreply_entry { * @param ar: ar count * @param total: total rrset count (presumably an+ns+ar). * @param sec: security status of the reply info. - * @param: reason_bogus: the Extended DNS Error for DNSSEC bogus status + * @param reason_bogus: the Extended DNS Error for DNSSEC bogus status * @return the reply_info base struct with the array for putting the rrsets * in. The array has been zeroed. Returns NULL on malloc failure. */ struct reply_info* construct_reply_info_base(struct regional* region, uint16_t flags, size_t qd, - time_t ttl, time_t prettl, time_t expttl, size_t an, size_t ns, - size_t ar, size_t total, enum sec_status sec, - sldns_ede_code reason_bogus); + time_t ttl, time_t prettl, time_t expttl, size_t an, size_t ns, + size_t ar, size_t total, enum sec_status sec, + sldns_ede_code reason_bogus); /** * Parse wire query into a queryinfo structure, return 0 on parse error. diff --git a/validator/validator.c b/validator/validator.c index f4c8242a8..18e4de072 100644 --- a/validator/validator.c +++ b/validator/validator.c @@ -2154,20 +2154,14 @@ processFinished(struct module_qstate* qstate, struct val_qstate* vq, char* err_str = errinf_to_str_bogus(qstate); if(err_str) { size_t err_str_len = strlen(err_str); - - /* allocate space and store the error string and it's size*/ - if (qstate->region) { - vq->orig_msg->rep->reason_bogus_str = regional_alloc( - qstate->region, - sizeof(char) * (err_str_len + 1)); - } else { - vq->orig_msg->rep->reason_bogus_str = malloc( - sizeof(char) * (err_str_len + 1)); - } - + log_info("%s", err_str); + /* allocate space and store the error + * string; */ + vq->orig_msg->rep->reason_bogus_str = regional_alloc( + qstate->region, + sizeof(char) * (err_str_len+1)); memcpy(vq->orig_msg->rep->reason_bogus_str, - err_str, err_str_len + 1); - vq->orig_msg->rep->reason_bogus_str_size = err_str_len; + err_str, err_str_len+1); } free(err_str); }