BUG/MEDIUM: dns: Don't store additional records in a linked-list
A SRV record keeps a reference on the corresponding additional record, if
any. But this additional record is also inserted in a separate linked-list into
the dns response. The problems arise when obsolete additional records are
released. The additional records list is purged but the SRV records always
reference these objects, leading to an undefined behavior. Worst, this happens
very quickly because additional records are never renewed. Thus, once received,
an additional record will always expire.
Now, the addtional record are only associated to a SRV record or simply
ignored. And the last version is always used.
This patch helps to fix the issue #841. It must be backported to 2.2.
diff --git a/include/haproxy/dns-t.h b/include/haproxy/dns-t.h
index b1b4397..8240372 100644
--- a/include/haproxy/dns-t.h
+++ b/include/haproxy/dns-t.h
@@ -155,7 +155,6 @@
struct dns_header header;
struct list query_list;
struct list answer_list;
- struct list ar_list; /* additional records */
/* authority ignored for now */
};
diff --git a/src/dns.c b/src/dns.c
index 3337802..8b0e659 100644
--- a/src/dns.c
+++ b/src/dns.c
@@ -523,15 +523,14 @@
struct server *srv;
struct dns_srvrq *srvrq;
- /* clean up obsolete Additional records */
- list_for_each_entry_safe(item, itemback, &res->response.ar_list, list) {
- if ((item->last_seen + resolvers->hold.obsolete / 1000) < now.tv_sec) {
- LIST_DEL(&item->list);
- pool_free(dns_answer_item_pool, item);
- }
- }
-
list_for_each_entry_safe(item, itemback, &res->response.answer_list, list) {
+ struct dns_answer_item *ar_item = item->ar_item;
+
+ /* clean up obsolete Additional record */
+ if (ar_item && (ar_item->last_seen + resolvers->hold.obsolete / 1000) < now.tv_sec) {
+ pool_free(dns_answer_item_pool, ar_item);
+ item->ar_item = NULL;
+ }
/* Remove obsolete items */
if ((item->last_seen + resolvers->hold.obsolete / 1000) < now.tv_sec) {
@@ -562,6 +561,10 @@
rm_obselete_item:
LIST_DEL(&item->list);
+ if (item->ar_item) {
+ pool_free(dns_answer_item_pool, item->ar_item);
+ item->ar_item = NULL;
+ }
pool_free(dns_answer_item_pool, item);
continue;
}
@@ -1213,17 +1216,17 @@
// looking for the SRV record in the response list linked to this additional record
list_for_each_entry(tmp_record, &dns_p->answer_list, list) {
- if ( !(
- (tmp_record->type == DNS_RTYPE_SRV) &&
- (tmp_record->ar_item == NULL) &&
- (dns_hostname_cmp(tmp_record->target, dns_answer_record->name, tmp_record->data_len) == 0)
- )
- )
- continue;
- tmp_record->ar_item = dns_answer_record;
+ if (tmp_record->type == DNS_RTYPE_SRV &&
+ !dns_hostname_cmp(tmp_record->target, dns_answer_record->name, tmp_record->data_len)) {
+ /* Always use the received additional record to refresh info */
+ if (tmp_record->ar_item)
+ pool_free(dns_answer_item_pool, tmp_record->ar_item);
+ tmp_record->ar_item = dns_answer_record;
+ break;
+ }
}
-
- LIST_ADDQ(&dns_p->ar_list, &dns_answer_record->list);
+ if (tmp_record->ar_item != dns_answer_record)
+ pool_free(dns_answer_item_pool, dns_answer_record);
dns_answer_record = NULL;
}
} /* for i 0 to arcount */
@@ -1592,7 +1595,6 @@
LIST_INIT(&res->requesters);
LIST_INIT(&res->response.answer_list);
- LIST_INIT(&res->response.ar_list);
res->prefered_query_type = query_type;
res->query_type = query_type;
@@ -1623,13 +1625,12 @@
req->resolution = NULL;
}
- list_for_each_entry_safe(item, itemback, &resolution->response.ar_list, list) {
- LIST_DEL(&item->list);
- pool_free(dns_answer_item_pool, item);
- }
-
list_for_each_entry_safe(item, itemback, &resolution->response.answer_list, list) {
LIST_DEL(&item->list);
+ if (item->ar_item) {
+ pool_free(dns_answer_item_pool, item->ar_item);
+ item->ar_item = NULL;
+ }
pool_free(dns_answer_item_pool, item);
}