Apply suggestions from @wtoorop's code review

Co-authored-by: Willem Toorop <willem@nlnetlabs.nl>

diff --git a/edns.c b/edns.c
index 099ea23244d8968e3bac91f8f4d9d0930559e437..2771ce0ec90435be663cee88ab3a4c56a7c829bd 100644 (file)
--- a/edns.c
+++ b/edns.c
@@ -126,7 +126,7 @@ ldns_edns_option_list_deep_free(ldns_edns_option_list *options_list)
 
     if (options_list) {
         for (i=0; i < ldns_edns_option_list_get_count(options_list); i++) {
-            ldns_edns_free(ldns_edns_option_list_get_option(options_list, i));
+            ldns_edns_deep_free(ldns_edns_option_list_get_option(options_list, i));
         }
         ldns_edns_option_list_free(options_list);
     }
@@ -218,25 +218,20 @@ ldns_edns_option_list_pop(ldns_edns_option_list *options_list)
 
     count = ldns_edns_option_list_get_count(options_list);
 
-    /* get the last option from the list */
-    pop = ldns_edns_option_list_get_option(options_list, count-1);
-
-    if (count <= 0){
+    if (count == 0){
         return NULL;
     }
+    /* get the last option from the list */
+    pop = ldns_edns_option_list_get_option(options_list, count-1);
 
     // @TODO rethink reallocing per pop
 
     /* shrink the array */
     new_list = LDNS_XREALLOC(options_list->_options, ldns_edns_option *, count -1);
-    if (new_list == NULL){
-        return NULL;
-        // @TODO not sure about returning NULL here, as ldns_rr_list_pop_rr
-        // just skips the failure
+    if (new_list){
+        options_list->_options = new_list;
     }
 
-    options_list->_options = new_list;
-
     ldns_edns_option_list_set_count(options_list, count - 1);
 
     return pop;

diff --git a/host2str.c b/host2str.c
index e0b9cfa30cce7052142fed12bee3472ead1a74d9..91df9f68c41ccc13a366bc70a1d7a0d70343e062 100644 (file)
--- a/host2str.c
+++ b/host2str.c
@@ -2661,8 +2661,6 @@ ldns_pkt2buffer_str_fmt(ldns_buffer *output,
                        ldns_buffer_printf(output, " ; udp: %u\n",
                                           ldns_pkt_edns_udp_size(pkt));
 
-                       // @TODO make old output configurable?
-
                        if (ldns_pkt_edns_data(pkt)) {
                                ldns_edns_option_list* edns_list;
                                // ldns_buffer_printf(output, ";; Data: ");

diff --git a/ldns/edns.h b/ldns/edns.h
index 5fbd9f493d0001a743a99770417c72231da170bf..94fd60db368df94e96a6b5715a0b612128c88f6d 100644 (file)
--- a/ldns/edns.h
+++ b/ldns/edns.h
@@ -25,7 +25,7 @@ extern "C" {
  */
 enum ldns_enum_edns_option
 {
-    LDNS_EDNS_LLQ = 1, /* http://files.dns-sd.org/draft-sekar-dns-llq.txt */
+    LDNS_EDNS_LLQ = 1, /* RFC8764 */
     LDNS_EDNS_UL = 2, /* http://files.dns-sd.org/draft-sekar-dns-ul.txt */
     LDNS_EDNS_NSID = 3, /* RFC5001 */
     /* 4 draft-cheshire-edns0-owner-option */
@@ -33,10 +33,15 @@ enum ldns_enum_edns_option
     LDNS_EDNS_DHU = 6, /* RFC6975 */
     LDNS_EDNS_N3U = 7, /* RFC6975 */
     LDNS_EDNS_CLIENT_SUBNET = 8, /* RFC7871 */
-    LDNS_EDNS_KEEPALIVE = 11, /* draft-ietf-dnsop-edns-tcp-keepalive*/
+    LDNS_EDNS_EXPIRE = 9, /* RFC7314 */
+    LDNS_EDNS_COOKIE = 10, /* RFC7873 */
+    LDNS_EDNS_KEEPALIVE = 11, /* RFC7828*/
     LDNS_EDNS_PADDING = 12, /* RFC7830 */
+    LDNS_EDNS_CHAIN = 13, /* RFC7901 */
+    LDNS_EDNS_KEY_TAG = 14, /* RFC8145 */
     LDNS_EDNS_EDE = 15, /* RFC8914 */
     LDNS_EDNS_CLIENT_TAG = 16 /* draft-bellis-dnsop-edns-tags-01 */
+    LDNS_EDNS_SERVER_TAG = 17 /* draft-bellis-dnsop-edns-tags-01 */
 };
 typedef enum ldns_enum_edns_option ldns_edns_option_code;
 
@@ -69,7 +74,9 @@ enum ldns_edns_enum_ede_code
     LDNS_EDE_NOT_SUPPORTED = 21,
     LDNS_EDE_NO_REACHABLE_AUTHORITY = 22,
     LDNS_EDE_NETWORK_ERROR = 23,
-    LDNS_EDE_INVALID_DATA = 24
+    LDNS_EDE_INVALID_DATA = 24,
+    LDNS_EDE_SIGNATURE_EXPIRED_BEFORE_VALID = 25,
+    LDNS_EDE_TOO_EARLY = 26
 };
 typedef enum ldns_edns_enum_ede_code ldns_edns_ede_code;
 
@@ -118,7 +125,7 @@ typedef struct ldns_struct_edns_option_list ldns_edns_option_list;
 size_t ldns_edns_get_size(const ldns_edns_option *edns);
 
 /**
- * returns the size of the EDNS data.
+ * returns the option code of the EDNS data.
  * \param[in] *edns the EDNS struct to read from
  * \return uint16_t with the size
  */
@@ -139,7 +146,7 @@ uint8_t *ldns_edns_get_data(const ldns_edns_option *edns);
  * This function DOES NOT copy the contents from the buffer
  * \param[in] code the EDNS code
  * \param[in] size size of the buffer
- * \param[in] data pointer to the buffer to be copied
+ * \param[in] data pointer to the buffer to be assigned
  * \return the new EDNS structure or NULL on failure
  */
 ldns_edns_option *ldns_edns_new(ldns_edns_option_code code, size_t size, void *data);

diff --git a/ldns/packet.h b/ldns/packet.h
index f6ca360c0732f683f6784ef9d2c2a84da84bd751..5bb0df50e5339c6399cad9566e4d4047b117b929 100644 (file)
--- a/ldns/packet.h
+++ b/ldns/packet.h
@@ -732,7 +732,7 @@ bool ldns_pkt_edns(const ldns_pkt *packet);
 /**
  * Returns a list of structured EDNS options
  *
- * \param[in] packet the packet which contains the parsed EDNS data
+ * \param[in] packet the packet which contains the EDNS data
  * \return list of ldns_edns_option structs
  */
 ldns_edns_option_list* ldns_pkt_edns_option_list(const ldns_pkt *packet);

diff --git a/packet.c b/packet.c
index 27e711b780996314cba2256575467035185dda23..9dc3281548deaebf855ce25338c8b3902e4ce408 100644 (file)
--- a/packet.c
+++ b/packet.c
@@ -753,10 +753,14 @@ ldns_pkt_edns_option_list(const ldns_pkt *packet)
 {
        size_t pos = 0;
        ldns_edns_option_list* edns_list;
-       size_t max = ldns_rdf_size(ldns_pkt_edns_data(packet)); // @TODO is this ugly?
-       const uint8_t* wire = ldns_rdf_data(ldns_pkt_edns_data(packet)); // @TODO is this ugly?
-
+       size_t max;
+       const uint8_t* wire;
 
+       if (!ldns_pkt_edns_data(packet))
+               return NULL;
+               
+       max = ldns_rdf_size(ldns_pkt_edns_data(packet));
+       wire = ldns_rdf_data(ldns_pkt_edns_data(packet));
        // @TODO do checks so we don't read into un-auth memory (max !< 4)
 
 
@@ -768,28 +772,33 @@ ldns_pkt_edns_option_list(const ldns_pkt *packet)
        while (pos < max) {
                ldns_edns_option* edns;
                uint8_t *data;
+               if (pos + 4 > max) {
+                       ldns_edns_option_list_deep_free(edns_list);
+                       return NULL;
+               }
                ldns_edns_option_code code = ldns_read_uint16(&wire[pos]);
                size_t size = ldns_read_uint16(&wire[pos+2]);
                pos += 4;
-
+               if (pos + size > max) {
+                       ldns_edns_option_list_deep_free(edns_list);
+                       return NULL;
+               }
                data = LDNS_XMALLOC(uint8_t, size);
 
-               // @TODO think about commented-out code
-               // if (!data) {
-                       // status = LDNS_STATUS_MEM_ERR;
-                       // goto status_error;
-                       // printf("HERE: DATA == NULL\n");
-               // }
+               if (!data) {
+                       ldns_edns_option_list_deep_free(edns_list);
+                       return NULL;
+               }
                memcpy(data, &wire[pos], size);
                pos += size;
 
                edns = ldns_edns_new(code, size, data);
 
-               if (edns != NULL) {
-                       ldns_edns_option_list_push(edns_list, edns);
+               if (!edns) {
+                       ldns_edns_option_list_deep_free(edns_list);
+                       return NULL;
                }
-
-               // @TODO what do we do in case of edns == NULL?
+               ldns_edns_option_list_push(edns_list, edns);
 
        }
 

diff --git a/wire2host.c b/wire2host.c
index 77f6cbf2079f2b28f4fd523a9abc8cc39474b002..63b67a0d1713499eadec4a090f00c71a30b7d759 100644 (file)
--- a/wire2host.c
+++ b/wire2host.c
@@ -185,7 +185,7 @@ ldns_wire2rdf(ldns_rr *rr, const uint8_t *wire, size_t max, size_t *pos)
        end = *pos + (size_t) rd_length;
 
        rdf_index = 0;
-       while (*pos < end && 
+       while (*pos < end &&
                        rdf_index < ldns_rr_descriptor_maximum(descriptor)) {
 
                cur_rdf_length = 0;
@@ -468,7 +468,7 @@ ldns_wire2pkt(ldns_pkt **packet_p, const uint8_t *wire, size_t max)
                        ldns_pkt_set_edns_version(packet, data[1]);
                        ldns_pkt_set_edns_z(packet, ldns_read_uint16(&data[2]));
                        /* edns might not have rdfs */
-                       if (ldns_rr_get_type(rr) == LDNS_RR_TYPE_OPT) {
+                       if (ldns_rr_rdf(rr, 0)) {
                                ldns_rdf_deep_free(ldns_pkt_edns_data(packet));
                                ldns_pkt_set_edns_data(packet, ldns_rdf_clone(ldns_rr_rdf(rr, 0)));
                        }