Merge branch 'work/wildcard/cancel'

Conflicts:
	src/lib/datasrc/tests/memory_datasrc_unittest.cc

src/lib/datasrc/memory_datasrc.cc

@@ -420,10 +420,51 @@ struct MemoryZone::MemoryZoneImpl {
                     return (FindResult(DELEGATION, prepareRRset(name,
                         state.rrset_, rename)));
                 }
+
+                // If the RBTree search stopped at a node for a super domain
+                // of the search name, it means the search name exists in
+                // the zone but is empty.  Treat it as NXRRSET.
+                if (node_path.getLastComparisonResult().getRelation() ==
+                    NameComparisonResult::SUPERDOMAIN) {
+                    return (FindResult(NXRRSET, ConstRRsetPtr()));
+                }
+
                 /*
                  * No redirection anywhere. Let's try if it is a wildcard.
+                 *
+                 * The wildcard is checked after the empty non-terminal domain
+                 * case above, because if that one triggers, it means we should
+                 * not match according to 4.3.3 of RFC 1034 (the query name
+                 * is known to exist).
                  */
                 if (node->getFlag(DOMAINFLAG_WILD)) {
+                    /* Should we cancel this match?
+                     *
+                     * If we compare with some node and get a common ancestor,
+                     * it might mean we are comparing with a non-wildcard node.
+                     * In that case, we check which part is common. If we have
+                     * something in common that lives below the node we got
+                     * (the one above *), then we should cancel the match
+                     * according to section 4.3.3 of RFC 1034 (as the name
+                     * between the wildcard domain and the query name is known
+                     * to exist).
+                     *
+                     * Because the way the tree stores relative names, we will
+                     * have exactly one common label (the ".") in case we have
+                     * nothing common under the node we got and we will get
+                     * more common labels otherwise (yes, this relies on the
+                     * internal RBTree structure, which leaks out through this
+                     * little bit).
+                     *
+                     * If the empty non-terminal node actually exists in the
+                     * tree, then this cancellation is not needed, because we
+                     * will not get here at all.
+                     */
+                    if (node_path.getLastComparisonResult().getRelation() ==
+                        NameComparisonResult::COMMONANCESTOR && node_path.
+                        getLastComparisonResult().getCommonLabels() > 1) {
+                        return (FindResult(NXDOMAIN, ConstRRsetPtr()));
+                    }
                     Name wildcard(Name("*").concatenate(
                         node_path.getAbsoluteName()));
                     DomainTree::Result result(domains_.find(wildcard, &node));
@@ -442,14 +483,6 @@ struct MemoryZone::MemoryZoneImpl {
                     break;
                 }
 
-                // If the RBTree search stopped at a node for a super domain
-                // of the search name, it means the search name exists in
-                // the zone but is empty.  Treat it as NXRRSET.
-                if (node_path.getLastComparisonResult().getRelation() ==
-                    NameComparisonResult::SUPERDOMAIN) {
-                    return (FindResult(NXRRSET, ConstRRsetPtr()));
-                }
-
                 // fall through
             case DomainTree::NOTFOUND:
                 return (FindResult(NXDOMAIN, ConstRRsetPtr()));

src/lib/datasrc/tests/memory_datasrc_unittest.cc

@@ -205,6 +205,9 @@ public:
             {"*.dnamewild.example.org. 300 IN DNAME dnamewild.example.",
              &rr_dnamewild_},
             {"*.child.example.org. 300 IN A 192.0.2.1", &rr_child_wild_},
+            {"bar.foo.wild.example.org. 300 IN A 192.0.2.2", &rr_not_wild_},
+            {"baz.foo.wild.example.org. 300 IN A 192.0.2.3",
+             &rr_not_wild_another_},
             {NULL, NULL}
         };
 
@@ -256,6 +259,8 @@ public:
     RRsetPtr rr_nswild_, rr_dnamewild_;
     RRsetPtr rr_child_wild_;
     RRsetPtr rr_under_wild_;
+    RRsetPtr rr_not_wild_;
+    RRsetPtr rr_not_wild_another_;
 
     /**
      * \brief Test one find query to the zone.
@@ -298,6 +303,11 @@ public:
                 if (check_answer) {
                     EXPECT_EQ(answer, find_result.rrset);
                 } else if (check_wild_answer) {
+                    ASSERT_NE(ConstRRsetPtr(), answer) <<
+                        "Wrong test, don't check for wild names if you expect "
+                        "empty answer";
+                    ASSERT_NE(ConstRRsetPtr(), find_result.rrset) <<
+                        "No answer found";
                     RdataIteratorPtr expectedIt(answer->getRdataIterator());
                     RdataIteratorPtr actualIt(
                         find_result.rrset->getRdataIterator());
@@ -323,6 +333,8 @@ public:
                 }
             });
     }
+    // Internal part of the cancelWildcard test that is multiple times
+    void doCancelWildcardTest();
 };
 
 /**
@@ -885,6 +897,81 @@ TEST_F(MemoryZoneTest, nestedEmptyWildcard) {
     }
 }
 
+// We run this part twice from the below test, in two slightly different
+// situations
+void
+MemoryZoneTest::doCancelWildcardTest() {
+    // These should be canceled
+    {
+        SCOPED_TRACE("Canceled under foo.wild.example.org");
+        findTest(Name("aaa.foo.wild.example.org"), RRType::A(),
+            Zone::NXDOMAIN);
+        findTest(Name("zzz.foo.wild.example.org"), RRType::A(),
+            Zone::NXDOMAIN);
+    }
+
+    // This is existing, non-wildcard domain, shouldn't wildcard at all
+    {
+        SCOPED_TRACE("Existing domain under foo.wild.example.org");
+        findTest(Name("bar.foo.wild.example.org"), RRType::A(), Zone::SUCCESS,
+            true, rr_not_wild_);
+    }
+
+    // These should be caught by the wildcard
+    {
+        SCOPED_TRACE("Neighbor wildcards to foo.wild.example.org");
+
+        const char* names[] = {
+            "aaa.bbb.wild.example.org",
+            "aaa.zzz.wild.example.org",
+            "zzz.wild.example.org",
+            NULL
+        };
+
+        for (const char** name(names); *name != NULL; ++ name) {
+            SCOPED_TRACE(string("Node ") + *name);
+
+            findTest(Name(*name), RRType::A(), Zone::SUCCESS, false, rr_wild_,
+                NULL, NULL, Zone::FIND_DEFAULT, true);
+        }
+    }
+
+    // This shouldn't be wildcarded, it's an existing domain
+    {
+        SCOPED_TRACE("The foo.wild.example.org itself");
+        findTest(Name("foo.wild.example.org"), RRType::A(), Zone::NXRRSET);
+    }
+}
+
+/*
+ * This tests that if there's a name between the wildcard domain and the
+ * searched one, it will not trigger wildcard, for example, if we have
+ * *.wild.example.org and bar.foo.wild.example.org, then we know
+ * foo.wild.example.org exists and is not wildcard. Therefore, search for
+ * aaa.foo.wild.example.org should return NXDOMAIN.
+ *
+ * Tests few cases "around" the canceled wildcard match, to see something that
+ * shouldn't be canceled isn't.
+ */
+TEST_F(MemoryZoneTest, cancelWildcard) {
+    EXPECT_EQ(SUCCESS, zone_.add(rr_wild_));
+    EXPECT_EQ(SUCCESS, zone_.add(rr_not_wild_));
+
+    {
+        SCOPED_TRACE("Runnig with single entry under foo.wild.example.org");
+        doCancelWildcardTest();
+    }
+
+    // Try putting another one under foo.wild....
+    // The result should be the same but it will be done in another way in the
+    // code, because the foo.wild.example.org will exist in the tree.
+    EXPECT_EQ(SUCCESS, zone_.add(rr_not_wild_another_));
+    {
+        SCOPED_TRACE("Runnig with two entries under foo.wild.example.org");
+        doCancelWildcardTest();
+    }
+}
+
 TEST_F(MemoryZoneTest, loadBadWildcard) {
     // We reject loading the zone if it contains a wildcard name for
     // NS or DNAME.