MEDIUM: resolvers: No longer store query items in a list into the response
When the response is parsed, query items are stored in a list, attached to
the parsed response (resolve_response).
First, there is one and only one query sent at a time. Thus, there is no
reason to use a list. There is a test to be sure there is only one query
item in the response. Then, the reference on this query item is only used to
validate the domain name is the one requested. So the query list can be
removed. We only expect one query item, no reason to loop on query records.
In addition, the query domain name is now immediately checked against the
resolution domain name. This way, the query item is only manipulated during
the response parsing.
diff --git a/include/haproxy/resolvers-t.h b/include/haproxy/resolvers-t.h
index 2e53d91..dc1381b 100644
--- a/include/haproxy/resolvers-t.h
+++ b/include/haproxy/resolvers-t.h
@@ -97,7 +97,6 @@
char name[DNS_MAX_NAME_SIZE+1]; /* query name */
unsigned short type; /* question type */
unsigned short class; /* query class */
- struct list list;
};
/* NOTE: big endian structure */
@@ -124,7 +123,6 @@
struct resolv_response {
struct dns_header header;
- struct list query_list;
struct eb_root answer_tree;
/* authority ignored for now */
};
diff --git a/src/resolvers.c b/src/resolvers.c
index a583e28..3090cb4 100644
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -889,7 +889,6 @@
unsigned char *reader;
char *previous_dname, tmpname[DNS_MAX_NAME_SIZE];
int len, flags, offset;
- int query_record_id;
int nb_saved_records;
struct resolv_query_item *query;
struct resolv_answer_item *answer_record, *tmp_record;
@@ -987,49 +986,41 @@
r_res->header.arcount = reader[0] * 256 + reader[1];
reader += 2;
- /* Parsing dns queries */
- BUG_ON(!LIST_ISEMPTY(&r_res->query_list));
- for (query_record_id = 0; query_record_id < r_res->header.qdcount; query_record_id++) {
- /* Use next pre-allocated resolv_query_item after ensuring there is
- * still one available.
- * It's then added to our packet query list. */
- if (query_record_id > DNS_MAX_QUERY_RECORDS)
- goto invalid_resp;
- query = &resolution->response_query_records[query_record_id];
- LIST_APPEND(&r_res->query_list, &query->list);
+ /* Parsing dns queries. For now there is only one query and it exists
+ * because (qdcount == 1).
+ */
+ query = &resolution->response_query_records[0];
- /* Name is a NULL terminated string in our case, since we have
- * one query per response and the first one can't be compressed
- * (using the 0x0c format) */
- offset = 0;
- len = resolv_read_name(resp, bufend, reader, query->name, DNS_MAX_NAME_SIZE, &offset, 0);
+ /* Name is a NULL terminated string in our case, since we have
+ * one query per response and the first one can't be compressed
+ * (using the 0x0c format) */
+ offset = 0;
+ len = resolv_read_name(resp, bufend, reader, query->name, DNS_MAX_NAME_SIZE, &offset, 0);
- if (len == 0)
- goto invalid_resp;
+ if (len == 0)
+ goto invalid_resp;
- reader += offset;
- previous_dname = query->name;
+ /* Now let's check the query's dname corresponds to the one we sent. */
+ if (len != resolution->hostname_dn_len ||
+ memcmp(query->name, resolution->hostname_dn, resolution->hostname_dn_len) != 0) {
+ cause = RSLV_RESP_WRONG_NAME;
+ goto invalid_resp;
+ }
- /* move forward 2 bytes for question type */
- if (reader + 2 >= bufend)
- goto invalid_resp;
- query->type = reader[0] * 256 + reader[1];
- reader += 2;
+ reader += offset;
+ previous_dname = query->name;
- /* move forward 2 bytes for question class */
- if (reader + 2 >= bufend)
- goto invalid_resp;
- query->class = reader[0] * 256 + reader[1];
- reader += 2;
- }
+ /* move forward 2 bytes for question type */
+ if (reader + 2 >= bufend)
+ goto invalid_resp;
+ query->type = reader[0] * 256 + reader[1];
+ reader += 2;
- /* Let's just make gcc happy. The tests above make it clear that
- * qdcount==1 hence that we necessarily enter into the loop at least
- * once, but gcc seems to be having difficulties following it and
- * warns about the risk of NULL dereference at the next line, even
- * if a BUG_ON(!query) is used.
- */
- ALREADY_CHECKED(query);
+ /* move forward 2 bytes for question class */
+ if (reader + 2 >= bufend)
+ goto invalid_resp;
+ query->class = reader[0] * 256 + reader[1];
+ reader += 2;
/* TRUNCATED flag must be checked after we could read the query type
* because a TRUNCATED SRV query type response can still be exploited
@@ -1478,8 +1469,6 @@
cause = RSLV_RESP_INVALID;
return_error:
- if (query)
- LIST_DEL_INIT(&query->list);
pool_free(resolv_answer_item_pool, answer_record);
return cause;
}
@@ -1852,8 +1841,6 @@
/* No resolution could be found, so let's allocate a new one */
res = pool_zalloc(resolv_resolution_pool);
if (res) {
- int i;
-
res->resolvers = resolvers;
res->uuid = resolution_uuid;
res->status = RSLV_STATUS_NONE;
@@ -1861,12 +1848,8 @@
res->last_valid = now_ms;
LIST_INIT(&res->requesters);
- LIST_INIT(&res->response.query_list);
res->response.answer_tree = EB_ROOT;
- for (i = 0; i < DNS_MAX_QUERY_RECORDS; i++)
- LIST_INIT(&res->response_query_records[i].list);
-
res->prefered_query_type = query_type;
res->query_type = query_type;
res->hostname_dn = *hostname_dn;
@@ -1896,15 +1879,6 @@
}
}
-/* deletes all query_items from the resolution's query_list */
-static void resolv_purge_resolution_query_items(struct resolv_resolution *resolution)
-{
- struct resolv_query_item *item, *itemback;
-
- list_for_each_entry_safe(item, itemback, &resolution->response.query_list, list)
- LIST_DEL_INIT(&item->list);
-}
-
/* Releases a resolution from its requester(s) and move it back to the pool */
static void resolv_free_resolution(struct resolv_resolution *resolution)
{
@@ -1920,7 +1894,6 @@
req->resolution = NULL;
}
resolv_purge_resolution_answer_records(resolution);
- resolv_purge_resolution_query_items(resolution);
LIST_DEL_INIT(&resolution->list);
pool_free(resolv_resolution_pool, resolution);
@@ -2147,7 +2120,6 @@
struct dns_counters *tmpcounters;
struct resolvers *resolvers;
struct resolv_resolution *res;
- struct resolv_query_item *query;
unsigned char buf[DNS_MAX_UDP_MESSAGE + 1];
unsigned char *bufend;
int buflen, dns_resp;
@@ -2274,20 +2246,6 @@
continue;
}
- /* Now let's check the query's dname corresponds to the one we
- * sent. We can check only the first query of the list. We send
- * one query at a time so we get one query in the response.
- */
- if (!LIST_ISEMPTY(&res->response.query_list)) {
- query = LIST_NEXT(&res->response.query_list, struct resolv_query_item *, list);
- LIST_DEL_INIT(&query->list);
- if (memcmp(query->name, res->hostname_dn, res->hostname_dn_len) != 0) {
- dns_resp = RSLV_RESP_WRONG_NAME;
- ns->counters->app.resolver.other++;
- goto report_res_error;
- }
- }
-
/* So the resolution succeeded */
res->status = RSLV_STATUS_VALID;
res->last_valid = now_ms;