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 6041b43d39264ddf335a79bbb5e101e4f8a8fa67..564cd4992dd739c9659c876a7a4a18e5da7c98b3 100644 (file)
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -926,7 +926,7 @@ static int resolv_validate_dns_response(unsigned char *resp, unsigned char *bufe
        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 @@ static struct resolv_resolution *resolv_pick_resolution(struct resolvers *resolv
        /* 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 @@ static struct resolv_resolution *resolv_pick_resolution(struct resolvers *resolv
                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 @@ static void resolv_purge_resolution_answer_records(struct resolv_resolution *res
        }
 }
 
+/* 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 @@ static void resolv_free_resolution(struct resolv_resolution *resolution)
                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 @@ static int resolv_process_responses(struct dns_nameserver *ns)
 
                /* 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 */