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.

diff --git a/NEWS b/NEWS
index 33827c7c2939d3f049fd1b0a3a8ab4f51ae91bc6..3da0b22c486036a64f9a351f6877f83dab91cfc3 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -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)
 ================================

diff --git a/lib/utils.c b/lib/utils.c
index 99d9398eb64d7fe9174f28b870f0f2f6ad485b6f..5283b13f25c2dd0be3f7c1c6d071cc2dc23040ad 100644 (file)
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -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();

diff --git a/lib/utils.h b/lib/utils.h
index b846dbdb0b10ddaf06ab10b335329a946215abde..2f037c26ddb9c1cd6e3753b73a69ab39de4a7385 100644 (file)
--- a/lib/utils.h
+++ b/lib/utils.h
@@ -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 */