BUG/MEDIUM: resolvers: always check a valid item in query_list
The query_list is physically stored in the struct resolution itself,
so we have a list that contains a list to items stored in itself (and
there is a single item). But the list is first initialized in
resolv_validate_dns_response(), while it's scanned in
resolv_process_responses() later after calling the former. First,
this results in crashes as soon as the code is instrumented a little
bit for debugging, as elements from a previous incarnation can appear.
But in addition to this, the presence of an element is checked by
verifying that the return of LIST_NEXT() is not NULL, while it may
never be NULL even for an empty list, resulting in bugs or crashes
if the number of responses does not match the list's contents. This
is easily triggered by testing for the list non-emptiness outside of
the function.
Let's make sure the list is always correct, i.e. it's initialized to
an empty list when the structure is allocated, elements are checked by
first verifying the list is not empty, they are deleted once checked,
and in any case at end so that there are no dangling pointers.
This should be backported, but only as long as the patch fits without
modifications, as adaptations can be risky there given that bugs tend
to hide each other.
diff --git a/src/resolvers.c b/src/resolvers.c
index 6041b43..564cd49 100644
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -926,7 +926,7 @@
reader += 2;
/* Parsing dns queries */
- LIST_INIT(&r_res->query_list);
+ 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.
@@ -1773,6 +1773,8 @@
/* 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;
@@ -1780,8 +1782,12 @@
res->last_valid = now_ms;
LIST_INIT(&res->requesters);
+ LIST_INIT(&res->response.query_list);
LIST_INIT(&res->response.answer_list);
+ 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;
@@ -1807,6 +1813,15 @@
}
}
+/* 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)
{
@@ -1822,6 +1837,8 @@
req->resolution = NULL;
}
resolv_purge_resolution_answer_records(resolution);
+ resolv_purge_resolution_query_items(resolution);
+
LIST_DELETE(&resolution->list);
pool_free(resolv_resolution_pool, resolution);
}
@@ -2160,12 +2177,16 @@
/* 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 */
- query = LIST_NEXT(&res->response.query_list, struct resolv_query_item *, list);
- if (query && memcmp(query->name, res->hostname_dn, res->hostname_dn_len) != 0) {
- dns_resp = RSLV_RESP_WRONG_NAME;
- ns->counters->other++;
- goto report_res_error;
+ * 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->other++;
+ goto report_res_error;
+ }
}
/* So the resolution succeeded */