Changes from codereview from CZ.NIC and Paul Wouters

- use of pkt pointer before test for NULL in ldns_dnssec_build_data_chain
- Memory leak on memory error in ldns_tsig_mac_new
- Smaller stack ocuupation in read_key_file in drill_util.c
- Potential for filedescriptor leak in ldns_init_random
- Memory leak on memory error in ldns_str2rdf_apl
- Memory leak (not freeing hexdata) in ldns_rr_new_frm_str_internal
- Memory leak when testing for parse in ldns_rr_new_frm_str_interal
- Memory leak when testing for parse in ldns_rr_new_frm_fp_l
- Memory leak on memory error in ldns_rr_list_sort
- Memory leak when popping zero items with ldns_rr_list_pop_rr_list

diff --git a/Changelog b/Changelog
index a8fe7fa6f734e3e82455ec4b8678e2d792cd0b0f..ff454158ed7bd0801fed05acf9c7cfde69c16594 100644 (file)
--- a/Changelog
+++ b/Changelog
@@ -1,4 +1,5 @@
 1.6.14
+       * Various bugfixes from code reviews from CZ.NIC and Paul Wouters
        * ldns-notify TSIG option argument checking
        * Let ldns_resolver_nameservers_randomize keep nameservers and rtt's
          in sync.

diff --git a/dnssec_verify.c b/dnssec_verify.c
index 1d93e7ba7b3ae6ecd99ccc99cab50795b8ca0082..1af7f1829889043d6c21db24afa555f17da88deb 100644 (file)
--- a/dnssec_verify.c
+++ b/dnssec_verify.c
@@ -285,6 +285,8 @@ ldns_dnssec_build_data_chain(ldns_resolver *res,
        ldns_rr_class c = 0;
 
        bool other_rrset = false;
+
+       assert(pkt != NULL);
        
        ldns_dnssec_data_chain *new_chain = ldns_dnssec_data_chain_new();
 

diff --git a/drill/drill_util.c b/drill/drill_util.c
index 98d88e7942da52acb5c345342acd641fa2491a55..a3753c82b3f51194692042300905a676e6af9985 100644 (file)
--- a/drill/drill_util.c
+++ b/drill/drill_util.c
@@ -40,7 +40,7 @@ read_key_file(const char *filename, ldns_rr_list *key_list)
        int line_len = 0;
        int line_nr = 0;
        int key_count = 0;
-       char line[LDNS_MAX_PACKETLEN];
+       char line[LDNS_MAX_LINELEN];
        ldns_status status;
        FILE *input_file;
        ldns_rr *rr;

diff --git a/rr.c b/rr.c
index 0254d4763bb3850e7402bef237137ef1de7f6da2..72076d40c58d972adf480d0fe42e0d0d6435950e 100644 (file)
--- a/rr.c
+++ b/rr.c
@@ -477,6 +477,7 @@ ldns_rr_new_frm_str_internal(ldns_rr **newrr, const char *str,
                                                                 ldns_buffer_free(rr_buf);
                                                                 LDNS_FREE(rdata);
                                                                 ldns_rr_free(new);
+                                                               LDNS_FREE(hex_data);
                                                                 return s;
                                                         }
                                                        LDNS_FREE(hex_data);
@@ -600,6 +601,9 @@ ldns_rr_new_frm_str_internal(ldns_rr **newrr, const char *str,
 
        if (newrr) {
                *newrr = new;
+       } else {
+               /* Maybe the caller just wanted to see if it would parse? */
+               ldns_rr_free(new);
        }
        return LDNS_STATUS_OK;
 
@@ -724,8 +728,13 @@ ldns_rr_new_frm_fp_l(ldns_rr **newrr, FILE *fp, uint32_t *default_ttl, ldns_rdf
                }
        }
        LDNS_FREE(line);
-       if (newrr && s == LDNS_STATUS_OK) {
-               *newrr = rr;
+       if (s == LDNS_STATUS_OK) {
+               if (newrr) {
+                       *newrr = rr;
+               } else {
+                       /* Just testing if it would parse? */
+                       ldns_rr_free(rr);
+               }
        }
        return s;
 }
@@ -1156,7 +1165,8 @@ ldns_rr_list_pop_rr_list(ldns_rr_list *rr_list, size_t howmany)
                i--;
        }
 
-       if (i == howmany) {
+       if (i == howmany) { /* so i <= 0 */
+               ldns_rr_list_free(popped);
                return NULL;
        } else {
                return popped;
@@ -1480,6 +1490,7 @@ ldns_rr_list_sort(ldns_rr_list *unsorted)
                                         LDNS_FREE(sortables[i]);
                                 }
                                 /* no way to return error */
+                               LDNS_FREE(sortables);
                                 return;
                         }
                        sortables[i]->original_object = ldns_rr_list_rr(unsorted, i);

diff --git a/str2host.c b/str2host.c
index 3553c0d54a56faf8a362327cd1971ea8e4323575..b5e4be10df769148a01f6891c7430bf979f0a8ac 100644 (file)
--- a/str2host.c
+++ b/str2host.c
@@ -534,6 +534,7 @@ ldns_str2rdf_apl(ldns_rdf **rd, const char *str)
 
        data = LDNS_XMALLOC(uint8_t, 4 + afdlength);
         if(!data) {
+               LDNS_FREE(afdpart);
                LDNS_FREE(my_ip_str);
                return LDNS_STATUS_INVALID_STR;
         }

diff --git a/tsig.c b/tsig.c
index f2f0a3f3f54abba5364feb2994d3c4c2ebbc5510..41693463df3a1eca1dfe0f4c0a0268333eb5b34a 100644 (file)
--- a/tsig.c
+++ b/tsig.c
@@ -179,10 +179,12 @@ ldns_tsig_mac_new(ldns_rdf **tsig_mac, uint8_t *pkt_wire, size_t pkt_wire_size,
                return LDNS_STATUS_NULL;
        }
        canonical_key_name_rdf  = ldns_rdf_clone(key_name_rdf);
+       if (canonical_key_name_rdf == NULL) {
+               return LDNS_STATUS_MEM_ERR;
+       }
        canonical_algorithm_rdf = ldns_rdf_clone(algorithm_rdf);
-
-       if (canonical_key_name_rdf == NULL 
-                       || canonical_algorithm_rdf  == NULL) {
+       if (canonical_algorithm_rdf == NULL) {
+               ldns_rdf_deep_free(canonical_key_name_rdf);
                return LDNS_STATUS_MEM_ERR;
        }
        /*

diff --git a/util.c b/util.c
index 475d23268a6fba136d4d42205c9448fb4dbf32c6..f5462c4b08f644ed4ad9b24296b289dbe21dbbf8 100644 (file)
--- a/util.c
+++ b/util.c
@@ -404,6 +404,7 @@ ldns_init_random(FILE *fd, unsigned int size)
 
        if (read < size) {
                LDNS_FREE(seed);
+               if (!fd) fclose(rand_f);
                return 1;
        } else {
 #ifdef HAVE_SSL