Commit 8d79b517 authored by Vladimír Čunát's avatar Vladimír Čunát

kr_ranked_rrarray*: avoid duplicate RRsets on wire

Fixes https://gitlab.labs.nic.cz/knot/resolver/issues/198.
We can't let multiple "matching RRsets" to the wire, and we can't just
merge the sets from multiple queries either.  The only way is to choose
either of the sets and put it on the wire.  ATM the last one wins.

Common ocurrence of the bug: if www.example.cz was a CNAME for example.cz
and we ask for a non-existent type, we would get the SOA record twice
in the final answer.

A few related changes:
 - don't just assert, also return error code if -DNDEBUG
 - kr_ranked_rrarray_set_wire: don't do full-content comparison anymore;
   see the first paragraph in this commit message for the reasons
 - minor refactoring of that code, more comments, etc.
parent 4fa31008
......@@ -21,6 +21,8 @@ Improvements
Bugfixes
--------
- validate: fix insufficient caching for some cases (relatively rare)
- avoid putting "duplicate" record-sets into the answer (#198)
Knot Resolver 1.2.6 (2017-04-24)
================================
......
......@@ -468,6 +468,51 @@ int kr_rrarray_add(rr_array_t *array, const knot_rrset_t *rr, knot_mm_t *pool)
return kr_ok();
}
/** Return whether two RRsets match, i.e. would form the same set; see ranked_rr_array_t */
static inline bool rrsets_match(const knot_rrset_t *rr1, const knot_rrset_t *rr2)
{
bool match = rr1->type == rr2->type && rr1->rclass == rr2->rclass;
if (match && rr2->type == KNOT_RRTYPE_RRSIG) {
match = match && knot_rrsig_type_covered(&rr1->rrs, 0)
== knot_rrsig_type_covered(&rr2->rrs, 0);
}
match = match && knot_dname_is_equal(rr1->owner, rr2->owner);
return match;
}
/** Ensure that an index in a ranked array won't cause "duplicate" RRsets on wire.
*
* Other entries that would form the same RRset get to_wire = false.
* See also rrsets_match.
*/
static int to_wire_ensure_unique(ranked_rr_array_t *array, size_t index)
{
bool ok = array && index < array->len;
if (!ok) {
assert(false);
return kr_error(EINVAL);
}
const struct ranked_rr_array_entry *e0 = array->at[index];
if (!e0->to_wire) {
return kr_ok();
}
for (ssize_t i = array->len - 1; i >= 0; --i) {
/* ^ iterate backwards, as the end is more likely in CPU caches */
struct ranked_rr_array_entry *ei = array->at[i];
if (ei->qry_uid == e0->qry_uid /* assumption: no duplicates within qry */
|| !ei->to_wire /* no use for complex comparison if @to_wire */
) {
continue;
}
if (rrsets_match(ei->rr, e0->rr)) {
ei->to_wire = false;
}
}
return kr_ok();
}
int kr_ranked_rrarray_add(ranked_rr_array_t *array, const knot_rrset_t *rr,
uint8_t rank, bool to_wire, uint32_t qry_uid, knot_mm_t *pool)
{
......@@ -483,22 +528,18 @@ int kr_ranked_rrarray_add(ranked_rr_array_t *array, const knot_rrset_t *rr,
if (stashed->qry_uid != qry_uid) {
break;
}
if (stashed->rr->rclass == rr->rclass &&
stashed->rr->type == rr->type &&
knot_dname_is_equal(stashed->rr->owner, rr->owner)) {
/* Don't merge RRSIGs covering different types.
* Cache-related code relies on that. */
if (rr->type == KNOT_RRTYPE_RRSIG &&
knot_rrsig_type_covered(&rr->rrs, 0)
!= knot_rrsig_type_covered(&stashed->rr->rrs, 0)) {
continue;
}
assert(stashed->rank == rank &&
stashed->cached == false &&
stashed->to_wire == to_wire);
/* Merge */
return knot_rdataset_merge(&stashed->rr->rrs, &rr->rrs, pool);
if (!rrsets_match(stashed->rr, rr)) {
continue;
}
/* Found the entry to merge with. Check consistency and merge. */
bool ok = stashed->rank == rank
&& !stashed->cached
&& stashed->to_wire == to_wire;
if (!ok) {
assert(false);
return kr_error(EEXIST);
}
return knot_rdataset_merge(&stashed->rr->rrs, &rr->rrs, pool);
}
/* No stashed rrset found, add */
......@@ -525,7 +566,8 @@ int kr_ranked_rrarray_add(ranked_rr_array_t *array, const knot_rrset_t *rr,
entry->yielded = false;
entry->to_wire = to_wire;
array_push(*array, entry);
return kr_ok();
return to_wire_ensure_unique(array, array->len - 1);
}
int kr_ranked_rrarray_set_wire(ranked_rr_array_t *array, bool to_wire,
......@@ -537,24 +579,9 @@ int kr_ranked_rrarray_set_wire(ranked_rr_array_t *array, bool to_wire,
continue;
}
entry->to_wire = to_wire;
if (!to_wire || !check_dups) {
continue;
}
knot_rrset_t *rr = entry->rr;
for (size_t j = 0; j < array->len; ++j) {
ranked_rr_array_entry_t *stashed = array->at[j];
if (stashed->qry_uid == qry_uid) {
continue;
}
if (stashed->rr->rclass != rr->rclass ||
stashed->rr->type != rr->type) {
continue;
}
bool is_equal = knot_rrset_equal(rr, stashed->rr,
KNOT_RRSET_COMPARE_WHOLE);
if (is_equal) {
stashed->to_wire = false;
}
if (check_dups) {
int ret = to_wire_ensure_unique(array, i);
if (ret) return ret;
}
}
return kr_ok();
......
......@@ -111,6 +111,7 @@ static inline long time_diff(struct timeval *begin, struct timeval *end) {
/** @cond internal Array types */
struct kr_context;
typedef array_t(knot_rrset_t *) rr_array_t;
struct ranked_rr_array_entry {
uint32_t qry_uid;
......@@ -122,6 +123,14 @@ struct ranked_rr_array_entry {
knot_rrset_t *rr;
};
typedef struct ranked_rr_array_entry ranked_rr_array_entry_t;
/** Array of RRsets coming from multiple queries; for struct kr_request.
*
* Notes:
* - RRSIGs are only considered to form an RRset when the types covered match;
* cache-related code relies on that!
* - RRsets from the same packet (qry_uid) get merged.
*/
typedef array_t(ranked_rr_array_entry_t *) ranked_rr_array_t;
/* @endcond */
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment