From: tcarpay <8014108+TCY16@users.noreply.github.com> Date: Tue, 5 Apr 2022 09:33:13 +0000 (+0200) Subject: Apply suggestions from @wtoorop's code review X-Git-Tag: 1.8.2-rc.1~3^2~26 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c6f6a3a1a9afda76fcaafe83a0588d6408ffb5c2;p=thirdparty%2Fldns.git Apply suggestions from @wtoorop's code review Co-authored-by: Willem Toorop --- diff --git a/edns.c b/edns.c index 099ea232..2771ce0e 100644 --- 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 e0b9cfa3..91df9f68 100644 --- 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 5fbd9f49..94fd60db 100644 --- 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 f6ca360c..5bb0df50 100644 --- 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 27e711b7..9dc32815 100644 --- 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 77f6cbf2..63b67a0d 100644 --- 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))); }