code review.

git-svn-id: file:///svn/unbound/trunk@2688 be551aaa-1e26-0410-a405-d3ace91eadb9

diff --git a/daemon/remote.c b/daemon/remote.c
index 38ca15c85cdbb4f2b64bf523090c981d3be9c890..f2e8f3ff94323aa542dd5eac77d9a40468e3a89b 100644 (file)
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1393,6 +1393,7 @@ parse_delegpt(SSL* ssl, char* args, uint8_t* nm, int allow_names)
                                }
                                if(!delegpt_add_ns_mlc(dp, n, 0)) {
                                        (void)ssl_printf(ssl, "error out of memory\n");
+                                       free(n);
                                        delegpt_free_mlc(dp);
                                        return NULL;
                                }
@@ -1442,7 +1443,6 @@ do_forward(SSL* ssl, struct worker* worker, char* args)
                        return;
                if(!forwards_add_zone(fwd, LDNS_RR_CLASS_IN, dp)) {
                        (void)ssl_printf(ssl, "error out of memory\n");
-                       delegpt_free_mlc(dp);
                        return;
                }
        }
@@ -1514,7 +1514,6 @@ do_forward_add(SSL* ssl, struct worker* worker, char* args)
        }
        if(!forwards_add_zone(fwd, LDNS_RR_CLASS_IN, dp)) {
                (void)ssl_printf(ssl, "error out of memory\n");
-               delegpt_free_mlc(dp);
                free(nm);
                return;
        }
@@ -1571,7 +1570,6 @@ do_stub_add(SSL* ssl, struct worker* worker, char* args)
                forwards_delete_stub_hole(fwd, LDNS_RR_CLASS_IN, nm);
                if(insecure) anchors_delete_insecure(worker->env.anchors,
                        LDNS_RR_CLASS_IN, nm);
-               delegpt_free_mlc(dp);
                free(nm);
                return;
        }

diff --git a/doc/Changelog b/doc/Changelog
index 77511edca1c74517a2d3bb4fbb378bc298b102f1..71409d0b70871a58784a262842494e5cecc28c69 100644 (file)
--- a/doc/Changelog
+++ b/doc/Changelog
@@ -1,3 +1,6 @@
+18 June 2012: Wouter
+       - code review.
+
 15 June 2012: Wouter
        - implement log-time-ascii on windows.
        - The key-cache bad key ttl is now 60 seconds.

diff --git a/iterator/iter_fwd.c b/iterator/iter_fwd.c
index f12531f4764aa6f481c8697ac92f3830d74158c2..17ca566746de2afa7f3804cb2593a3f69c601f18 100644 (file)
--- a/iterator/iter_fwd.c
+++ b/iterator/iter_fwd.c
@@ -250,10 +250,12 @@ read_forwards(struct iter_forwards* fwd, struct config_file* cfg)
        struct config_stub* s;
        for(s = cfg->forwards; s; s = s->next) {
                struct delegpt* dp;
-               if(!(dp=read_fwds_name(s)) ||
-                       !read_fwds_host(s, dp) ||
-                       !read_fwds_addr(s, dp))
+               if(!(dp=read_fwds_name(s)))
                        return 0;
+               if(!read_fwds_host(s, dp) || !read_fwds_addr(s, dp)) {
+                       delegpt_free_mlc(dp);
+                       return 0;
+               }
                /* set flag that parent side NS information is included.
                 * Asking a (higher up) server on the internet is not useful */
                /* the flag is turned off for 'forward-first' so that the

diff --git a/iterator/iter_hints.c b/iterator/iter_hints.c
index cfb9db7abb8cf420db6f22d536ceb3c253346397..09e4731135dd4b80bdb9f5ec974f8b3c671f2a99 100644 (file)
--- a/iterator/iter_hints.c
+++ b/iterator/iter_hints.c
@@ -126,32 +126,35 @@ compile_time_root_prime(int do_ip4, int do_ip6)
                return NULL;
        dp->has_parent_side_NS = 1;
       if(do_ip4) {
-       if(!ah(dp, "A.ROOT-SERVERS.NET.", "198.41.0.4"))        return 0;
-       if(!ah(dp, "B.ROOT-SERVERS.NET.", "192.228.79.201")) return 0;
-       if(!ah(dp, "C.ROOT-SERVERS.NET.", "192.33.4.12"))       return 0;
-       if(!ah(dp, "D.ROOT-SERVERS.NET.", "128.8.10.90"))       return 0;
-       if(!ah(dp, "E.ROOT-SERVERS.NET.", "192.203.230.10")) return 0;
-       if(!ah(dp, "F.ROOT-SERVERS.NET.", "192.5.5.241"))       return 0;
-       if(!ah(dp, "G.ROOT-SERVERS.NET.", "192.112.36.4"))      return 0;
-       if(!ah(dp, "H.ROOT-SERVERS.NET.", "128.63.2.53"))       return 0;
-       if(!ah(dp, "I.ROOT-SERVERS.NET.", "192.36.148.17"))     return 0;
-       if(!ah(dp, "J.ROOT-SERVERS.NET.", "192.58.128.30"))     return 0;
-       if(!ah(dp, "K.ROOT-SERVERS.NET.", "193.0.14.129"))      return 0;
-       if(!ah(dp, "L.ROOT-SERVERS.NET.", "199.7.83.42"))       return 0;
-       if(!ah(dp, "M.ROOT-SERVERS.NET.", "202.12.27.33"))      return 0;
+       if(!ah(dp, "A.ROOT-SERVERS.NET.", "198.41.0.4"))        goto failed;
+       if(!ah(dp, "B.ROOT-SERVERS.NET.", "192.228.79.201")) goto failed;
+       if(!ah(dp, "C.ROOT-SERVERS.NET.", "192.33.4.12"))       goto failed;
+       if(!ah(dp, "D.ROOT-SERVERS.NET.", "128.8.10.90"))       goto failed;
+       if(!ah(dp, "E.ROOT-SERVERS.NET.", "192.203.230.10")) goto failed;
+       if(!ah(dp, "F.ROOT-SERVERS.NET.", "192.5.5.241"))       goto failed;
+       if(!ah(dp, "G.ROOT-SERVERS.NET.", "192.112.36.4"))      goto failed;
+       if(!ah(dp, "H.ROOT-SERVERS.NET.", "128.63.2.53"))       goto failed;
+       if(!ah(dp, "I.ROOT-SERVERS.NET.", "192.36.148.17"))     goto failed;
+       if(!ah(dp, "J.ROOT-SERVERS.NET.", "192.58.128.30"))     goto failed;
+       if(!ah(dp, "K.ROOT-SERVERS.NET.", "193.0.14.129"))      goto failed;
+       if(!ah(dp, "L.ROOT-SERVERS.NET.", "199.7.83.42"))       goto failed;
+       if(!ah(dp, "M.ROOT-SERVERS.NET.", "202.12.27.33"))      goto failed;
       }
       if(do_ip6) {
-       if(!ah(dp, "A.ROOT-SERVERS.NET.", "2001:503:ba3e::2:30")) return 0;
-       if(!ah(dp, "D.ROOT-SERVERS.NET.", "2001:500:2d::d")) return 0;
-       if(!ah(dp, "F.ROOT-SERVERS.NET.", "2001:500:2f::f")) return 0;
-       if(!ah(dp, "H.ROOT-SERVERS.NET.", "2001:500:1::803f:235")) return 0;
-       if(!ah(dp, "I.ROOT-SERVERS.NET.", "2001:7fe::53")) return 0;
-       if(!ah(dp, "J.ROOT-SERVERS.NET.", "2001:503:c27::2:30")) return 0;
-       if(!ah(dp, "K.ROOT-SERVERS.NET.", "2001:7fd::1")) return 0;
-       if(!ah(dp, "L.ROOT-SERVERS.NET.", "2001:500:3::42")) return 0;
-       if(!ah(dp, "M.ROOT-SERVERS.NET.", "2001:dc3::35")) return 0;
+       if(!ah(dp, "A.ROOT-SERVERS.NET.", "2001:503:ba3e::2:30")) goto failed;
+       if(!ah(dp, "D.ROOT-SERVERS.NET.", "2001:500:2d::d")) goto failed;
+       if(!ah(dp, "F.ROOT-SERVERS.NET.", "2001:500:2f::f")) goto failed;
+       if(!ah(dp, "H.ROOT-SERVERS.NET.", "2001:500:1::803f:235")) goto failed;
+       if(!ah(dp, "I.ROOT-SERVERS.NET.", "2001:7fe::53")) goto failed;
+       if(!ah(dp, "J.ROOT-SERVERS.NET.", "2001:503:c27::2:30")) goto failed;
+       if(!ah(dp, "K.ROOT-SERVERS.NET.", "2001:7fd::1")) goto failed;
+       if(!ah(dp, "L.ROOT-SERVERS.NET.", "2001:500:3::42")) goto failed;
+       if(!ah(dp, "M.ROOT-SERVERS.NET.", "2001:dc3::35")) goto failed;
       }
        return dp;
+failed:
+       delegpt_free_mlc(dp);
+       return 0;
 }
 
 /** insert new hint info into hint structure */
@@ -253,17 +256,19 @@ read_stubs(struct iter_hints* hints, struct config_file* cfg)
        struct config_stub* s;
        struct delegpt* dp;
        for(s = cfg->stubs; s; s = s->next) {
-               if(!(dp=read_stubs_name(s)) ||
-                       !read_stubs_host(s, dp) ||
-                       !read_stubs_addr(s, dp))
+               if(!(dp=read_stubs_name(s)))
+                       return 0;
+               if(!read_stubs_host(s, dp) || !read_stubs_addr(s, dp)) {
+                       delegpt_free_mlc(dp);
                        return 0;
+               }
                /* the flag is turned off for 'stub-first' so that the
                 * last resort will ask for parent-side NS record and thus
                 * fallback to the internet name servers on a failure */
                dp->has_parent_side_NS = (uint8_t)!s->isfirst;
+               delegpt_log(VERB_QUERY, dp);
                if(!hints_insert(hints, LDNS_RR_CLASS_IN, dp, !s->isprime))
                        return 0;
-               delegpt_log(VERB_QUERY, dp);
        }
        return 1;
 }

diff --git a/services/listen_dnsport.c b/services/listen_dnsport.c
index 59ca1991eb12141cfc9fd1b442f546bc907c11ff..647cbe07ebd9841b668de96445fe64ea0b2a9b9c 100644 (file)
--- a/services/listen_dnsport.c
+++ b/services/listen_dnsport.c
@@ -323,6 +323,11 @@ create_udp_sock(int family, int socktype, struct sockaddr* addr,
                        log_err("setsockopt(..., IP_MTU_DISCOVER, "
                                "IP_PMTUDISC_DONT...) failed: %s",
                                strerror(errno));
+#    ifndef USE_WINSOCK
+                       close(s);
+#    else
+                       closesocket(s);
+#    endif
                        return -1;
                }
 #  elif defined(IP_DONTFRAG)
@@ -331,6 +336,11 @@ create_udp_sock(int family, int socktype, struct sockaddr* addr,
                        &off, (socklen_t)sizeof(off)) < 0) {
                        log_err("setsockopt(..., IP_DONTFRAG, ...) failed: %s",
                                strerror(errno));
+#    ifndef USE_WINSOCK
+                       close(s);
+#    else
+                       closesocket(s);
+#    endif
                        return -1;
                }
 #  endif /* IPv4 MTU */
@@ -408,9 +418,11 @@ create_tcp_accept_sock(struct addrinfo *addr, int v6only, int* noproto)
 #ifndef USE_WINSOCK
                log_err("setsockopt(.. SO_REUSEADDR ..) failed: %s",
                        strerror(errno));
+               close(s);
 #else
                log_err("setsockopt(.. SO_REUSEADDR ..) failed: %s",
                        wsa_strerror(WSAGetLastError()));
+               closesocket(s);
 #endif
                return -1;
        }
@@ -422,9 +434,11 @@ create_tcp_accept_sock(struct addrinfo *addr, int v6only, int* noproto)
 #ifndef USE_WINSOCK
                        log_err("setsockopt(..., IPV6_V6ONLY, ...) failed: %s",
                                strerror(errno));
+                       close(s);
 #else
                        log_err("setsockopt(..., IPV6_V6ONLY, ...) failed: %s",
                                wsa_strerror(WSAGetLastError()));
+                       closesocket(s);
 #endif
                        return -1;
                }
@@ -443,23 +457,32 @@ create_tcp_accept_sock(struct addrinfo *addr, int v6only, int* noproto)
                                (struct sockaddr_storage*)addr->ai_addr,
                                addr->ai_addrlen);
                }
+               close(s);
 #else
                log_err("can't bind socket: %s", 
                        wsa_strerror(WSAGetLastError()));
                log_addr(0, "failed address",
                        (struct sockaddr_storage*)addr->ai_addr,
                        addr->ai_addrlen);
+               closesocket(s);
 #endif
                return -1;
        }
        if(!fd_set_nonblock(s)) {
+#ifndef USE_WINSOCK
+               close(s);
+#else
+               closesocket(s);
+#endif
                return -1;
        }
        if(listen(s, TCP_BACKLOG) == -1) {
 #ifndef USE_WINSOCK
                log_err("can't listen: %s", strerror(errno));
+               close(s);
 #else
                log_err("can't listen: %s", wsa_strerror(WSAGetLastError()));
+               closesocket(s);
 #endif
                return -1;
        }
@@ -653,8 +676,14 @@ ports_create_if(const char* ifname, int do_auto, int do_udp, int do_tcp,
                        return 0;
                }
                /* getting source addr packet info is highly non-portable */
-               if(!set_recvpktinfo(s, hints->ai_family))
+               if(!set_recvpktinfo(s, hints->ai_family)) {
+#ifndef USE_WINSOCK
+                       close(s);
+#else
+                       closesocket(s);
+#endif
                        return 0;
+               }
                if(!port_insert(list, s, listen_type_udpancil)) {
 #ifndef USE_WINSOCK
                        close(s);

diff --git a/smallapp/unbound-control.c b/smallapp/unbound-control.c
index 58be7b7abfc02ea27b961e8d1e35debdb7828ef3..a5aa92bf93a69f9b2de794bb318fadc4b9524fd2 100644 (file)
--- a/smallapp/unbound-control.c
+++ b/smallapp/unbound-control.c
@@ -379,7 +379,8 @@ int main(int argc, char* argv[])
        if(!RAND_status()) {
                 /* try to seed it */
                 unsigned char buf[256];
-                unsigned int v, seed=(unsigned)time(NULL) ^ (unsigned)getpid();
+                unsigned int seed=(unsigned)time(NULL) ^ (unsigned)getpid();
+               unsigned int v = seed;
                 size_t i;
                 for(i=0; i<256/sizeof(v); i++) {
                         memmove(buf+i*sizeof(v), &v, sizeof(v));

diff --git a/util/config_file.c b/util/config_file.c
index 8ba79d2a29ca63f2e3a0cb8c090ebe8a3caf1c57..2f5e988cb7a4f537a5f2c5ab5b055620bde79468 100644 (file)
--- a/util/config_file.c
+++ b/util/config_file.c
@@ -1038,9 +1038,9 @@ static int isalldigit(const char* str, size_t l)
 int 
 cfg_parse_memsize(const char* str, size_t* res)
 {
-       size_t len = (size_t)strlen(str);
+       size_t len;
        size_t mult = 1;
-       if(!str || len == 0) {
+       if(!str || (len=(size_t)strlen(str)) == 0) {
                log_err("not a size: '%s'", str);
                return 0;
        }

diff --git a/util/data/msgparse.c b/util/data/msgparse.c
index a03f543e827b5c656969ee9ea329cbeea1fa4ca4..c51c4406d11cbe7243c868f9a889f4ac38512393 100644 (file)
--- a/util/data/msgparse.c
+++ b/util/data/msgparse.c
@@ -655,8 +655,10 @@ calc_size(ldns_buffer* pkt, uint16_t type, struct rr_parse* rr)
                                len = 0;
                                break;
                        case LDNS_RDF_TYPE_STR:
-                               if(pkt_len < 1)
+                               if(pkt_len < 1) {
+                                       /* NOTREACHED, due to 'while(>0)' */
                                        return 0; /* len byte exceeds rdata */
+                               }
                                len = ldns_buffer_current(pkt)[0] + 1;
                                break;
                        default:

diff --git a/util/tube.c b/util/tube.c
index 67294e056c4b5c2fab364645b930e5f43cb65367..28c51d79d16d22e05b881e13a42a6ccad72ba291 100644 (file)
--- a/util/tube.c
+++ b/util/tube.c
@@ -360,6 +360,7 @@ int tube_read_msg(struct tube* tube, uint8_t** buf, uint32_t* len,
                }
                d += r;
        }
+       log_assert(*len < 65536*2);
        *buf = (uint8_t*)malloc(*len);
        if(!*buf) {
                log_err("tube read out of memory");

diff --git a/validator/autotrust.c b/validator/autotrust.c
index 9896943245e44fe7c980eb06c0ec30b721aca0cf..a361ab04ba38e004b18a70ac21b2199912b2fde0 100644 (file)
--- a/validator/autotrust.c
+++ b/validator/autotrust.c
@@ -1851,6 +1851,7 @@ static void
 autr_tp_remove(struct module_env* env, struct trust_anchor* tp,
        struct ub_packed_rrset_key* dnskey_rrset)
 {
+       struct trust_anchor* del_tp;
        struct trust_anchor key;
        struct autr_point_data pd;
        time_t mold, mnew;
@@ -1876,19 +1877,24 @@ autr_tp_remove(struct module_env* env, struct trust_anchor* tp,
 
        /* take from tree. It could be deleted by someone else,hence (void). */
        lock_basic_lock(&env->anchors->lock);
-       (void)rbtree_delete(env->anchors->tree, &key);
+       del_tp = (struct trust_anchor*)rbtree_delete(env->anchors->tree, &key);
        mold = wait_probe_time(env->anchors);
        (void)rbtree_delete(&env->anchors->autr->probe, &key);
        mnew = wait_probe_time(env->anchors);
        anchors_init_parents_locked(env->anchors);
        lock_basic_unlock(&env->anchors->lock);
 
-       /* save on disk */
-       tp->autr->next_probe_time = 0; /* no more probing for it */
-       autr_write_file(env, tp);
+       /* if !del_tp then the trust point is no longer present in the tree,
+        * it was deleted by someone else, who will write the zonefile and
+        * clean up the structure */
+       if(del_tp) {
+               /* save on disk */
+               del_tp->autr->next_probe_time = 0; /* no more probing for it */
+               autr_write_file(env, del_tp);
 
-       /* delete */
-       autr_point_delete(tp);
+               /* delete */
+               autr_point_delete(del_tp);
+       }
        if(mold != mnew) {
                reset_worker_timer(env);
        }

diff --git a/validator/val_anchor.c b/validator/val_anchor.c
index 200bf5d97be610edcaeba285a6a4c56c1bf8f654..fa69df52d6807bf5e90cf3e7d435af43c197bddb 100644 (file)
--- a/validator/val_anchor.c
+++ b/validator/val_anchor.c
@@ -1246,6 +1246,7 @@ anchors_delete_insecure(struct val_anchors* anchors, uint16_t c,
        lock_basic_lock(&ta->lock);
        /* see if its really an insecure point */
        if(ta->keylist || ta->autr || ta->numDS || ta->numDNSKEY) {
+               lock_basic_unlock(&anchors->lock);
                lock_basic_unlock(&ta->lock);
                /* its not an insecure point, do not remove it */
                return;

diff --git a/validator/val_sigcrypt.c b/validator/val_sigcrypt.c
index 4c79c004d02c90b648d792ced2c5ee0f24326c47..32dbc0bd7474c3d4158dcce02c99550d46985c39 100644 (file)
--- a/validator/val_sigcrypt.c
+++ b/validator/val_sigcrypt.c
@@ -606,10 +606,14 @@ dnskeyset_verify_rrset(struct module_env* env, struct val_env* ve,
                                (uint8_t)rrset_get_sig_algo(rrset, i));
                }
        }
-       verbose(VERB_ALGO, "rrset failed to verify: no valid signatures for "
-               "%d algorithms", (int)algo_needs_num_missing(&needs));
        if(sigalg && (alg=algo_needs_missing(&needs)) != 0) {
+               verbose(VERB_ALGO, "rrset failed to verify: "
+                       "no valid signatures for %d algorithms",
+                       (int)algo_needs_num_missing(&needs));
                algo_needs_reason(env, alg, reason, "no signatures");
+       } else {
+               verbose(VERB_ALGO, "rrset failed to verify: "
+                       "no valid signatures");
        }
        return sec_status_bogus;
 }