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 c915c5b0b6af10fd8e104aa75effc174e19a4658..8d0466c49b2801edc01164f5937fec31c3d7d989 100644 (file)
--- 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 505616b39665b95f37fe60bf13459816c4151d4b..529b22ae4141976c699cf7322e478086469e1fb6 100644 (file)
--- 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 8fe8a0c9937ab95c81a90bed076da7827f909c97..d1f4bc9554e8f75facd6298e4d6fb0f0cba3b038 100644 (file)
--- 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 225a68c61eb58aeaa503b918b0dca46befb84865..a3d0292312eec38df29fde71a79d29e82aebe186 100644 (file)
--- 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 48fa730b5f04767c7960f5d64b088b66dfc1bbbc..44da22d785d96dbfac27e1a6aa33e3ef1cb0249e 100644 (file)
--- 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 b1884d023c555a5594e2665df023b7b1c55a2849..792387444d18a8266afa0ce8089f5d44ee7957a9 100644 (file)
--- 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; i<rep->rrset_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 9ee402f849336ba90300d3a8dce7613c6d504158..1339fd9cc37fa026040f7a76f6c2640d3332a8e0 100644 (file)
--- 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 f4c8242a8fef60ffc9540dd1c0a1c6dbc68c14cb..18e4de07223c9f11c3be259043ba9d715bd3b51e 100644 (file)
--- 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);
                        }