Merge pull request #703 from rthalley/cmp_fix

Fix #698 and #702, problems caused by _cmp() giving the wrong result in some cases

5 files changed, 180 insertions, 15 deletions

diff --git a/dns/dnssec.py b/dns/dnssec.py
index f09ecd6..6e9946f 100644
--- a/dns/dnssec.py
+++ b/dns/dnssec.py
@@ -404,13 +404,13 @@ def _validate_rrsig(rrset, rrsig, keys, origin=None, now=None):
     rrnamebuf = rrname.to_digestable()
     rrfixed = struct.pack('!HHI', rdataset.rdtype, rdataset.rdclass,
                           rrsig.original_ttl)
-    for rr in sorted(rdataset):
+    rdatas = [rdata.to_digestable(origin) for rdata in rdataset]
+    for rdata in sorted(rdatas):
         data += rrnamebuf
         data += rrfixed
-        rrdata = rr.to_digestable(origin)
-        rrlen = struct.pack('!H', len(rrdata))
+        rrlen = struct.pack('!H', len(rdata))
         data += rrlen
-        data += rrdata
+        data += rdata
 
     chosen_hash = _make_hash(rrsig.algorithm)
 
diff --git a/dns/rdata.py b/dns/rdata.py
index 0831c41..624063e 100644
--- a/dns/rdata.py
+++ b/dns/rdata.py
@@ -38,6 +38,22 @@ import dns.ttl
 
 _chunksize = 32
 
+# We currently allow comparisons for rdata with relative names for backwards
+# compatibility, but in the future we will not, as these kinds of comparisons
+# can lead to subtle bugs if code is not carefully written.
+#
+# This switch allows the future behavior to be turned on so code can be
+# tested with it.
+_allow_relative_comparisons = True
+
+
+class NoRelativeRdataOrdering(dns.exception.DNSException):
+    """An attempt was made to do an ordered comparison of one or more
+    rdata with relative names.  The only reliable way of sorting rdata
+    is to use non-relativized rdata.
+
+    """
+
 
 def _wordbreak(data, chunksize=_chunksize, separator=b' '):
     """Break a binary string into chunks of chunksize characters separated by
@@ -232,12 +248,42 @@ class Rdata:
         """Compare an rdata with another rdata of the same rdtype and
         rdclass.
 
-        Return < 0 if self < other in the DNSSEC ordering, 0 if self
-        == other, and > 0 if self > other.
-
+        For rdata with only absolute names:
+            Return < 0 if self < other in the DNSSEC ordering, 0 if self
+            == other, and > 0 if self > other.
+        For rdata with at least one relative names:
+            The rdata sorts before any rdata with only absolute names.
+            When compared with another relative rdata, all names are
+            made absolute as if they were relative to the root, as the
+            proper origin is not available.  While this creates a stable
+            ordering, it is NOT guaranteed to be the DNSSEC ordering.
+            In the future, all ordering comparisons for rdata with
+            relative names will be disallowed.
         """
-        our = self.to_digestable(dns.name.root)
-        their = other.to_digestable(dns.name.root)
+        try:
+            our = self.to_digestable()
+            our_relative = False
+        except dns.name.NeedAbsoluteNameOrOrigin:
+            if _allow_relative_comparisons:
+                our = self.to_digestable(dns.name.root)
+            our_relative = True
+        try:
+            their = other.to_digestable()
+            their_relative = False
+        except dns.name.NeedAbsoluteNameOrOrigin:
+            if _allow_relative_comparisons:
+                their = other.to_digestable(dns.name.root)
+            their_relative = True
+        if _allow_relative_comparisons:
+            if our_relative != their_relative:
+                # For the purpose of comparison, all rdata with at least one
+                # relative name is less than an rdata with only absolute names.
+                if our_relative:
+                    return -1
+                else:
+                    return 1
+        elif our_relative or their_relative:
+            raise NoRelativeRdataOrdering
         if our == their:
             return 0
         elif our > their:
@@ -250,14 +296,28 @@ class Rdata:
             return False
         if self.rdclass != other.rdclass or self.rdtype != other.rdtype:
             return False
-        return self._cmp(other) == 0
+        our_relative = False
+        their_relative = False
+        try:
+            our = self.to_digestable()
+        except dns.name.NeedAbsoluteNameOrOrigin:
+            our = self.to_digestable(dns.name.root)
+            our_relative = True
+        try:
+            their = other.to_digestable()
+        except dns.name.NeedAbsoluteNameOrOrigin:
+            their = other.to_digestable(dns.name.root)
+            their_relative = True
+        if our_relative != their_relative:
+            return False
+        return our == their
 
     def __ne__(self, other):
         if not isinstance(other, Rdata):
             return True
         if self.rdclass != other.rdclass or self.rdtype != other.rdtype:
             return True
-        return self._cmp(other) != 0
+        return not self.__eq__(other)
 
     def __lt__(self, other):
         if not isinstance(other, Rdata) or \
diff --git a/dns/zone.py b/dns/zone.py
index 9c3204b..d154928 100644
--- a/dns/zone.py
+++ b/dns/zone.py
@@ -733,10 +733,11 @@ class Zone(dns.transaction.TransactionManager):
                     continue
                 rrfixed = struct.pack('!HHI', rdataset.rdtype,
                                       rdataset.rdclass, rdataset.ttl)
-                for rr in sorted(rdataset):
-                    rrdata = rr.to_digestable(self.origin)
-                    rrlen = struct.pack('!H', len(rrdata))
-                    hasher.update(rrnamebuf + rrfixed + rrlen + rrdata)
+                rdatas = [rdata.to_digestable(self.origin)
+                          for rdata in rdataset]
+                for rdata in sorted(rdatas):
+                    rrlen = struct.pack('!H', len(rdata))
+                    hasher.update(rrnamebuf + rrfixed + rrlen + rdata)
         return hasher.digest()
 
     def compute_digest(self, hash_algorithm, scheme=DigestScheme.SIMPLE):
diff --git a/tests/test_rdata.py b/tests/test_rdata.py
index 05ec6ca..f87ff56 100644
--- a/tests/test_rdata.py
+++ b/tests/test_rdata.py
@@ -696,7 +696,96 @@ class RdataTestCase(unittest.TestCase):
                     rr = dns.rdata.from_text('IN', 'DNSKEY', input_variation)
                     new_text = rr.to_text(chunksize=chunksize)
                     self.assertEqual(output, new_text)
+                    
+    def test_relative_vs_absolute_compare_unstrict(self):
+        try:
+            saved = dns.rdata._allow_relative_comparisons
+            dns.rdata._allow_relative_comparisons = True
+            r1 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www.')
+            r2 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www')
+            self.assertFalse(r1 == r2)
+            self.assertTrue(r1 != r2)
+            self.assertFalse(r1 < r2)
+            self.assertFalse(r1 <= r2)
+            self.assertTrue(r1 > r2)
+            self.assertTrue(r1 >= r2)
+            self.assertTrue(r2 < r1)
+            self.assertTrue(r2 <= r1)
+            self.assertFalse(r2 > r1)
+            self.assertFalse(r2 >= r1)
+        finally:
+            dns.rdata._allow_relative_comparisons = saved
+
+    def test_relative_vs_absolute_compare_strict(self):
+        try:
+            saved = dns.rdata._allow_relative_comparisons
+            dns.rdata._allow_relative_comparisons = False
+            r1 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www.')
+            r2 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www')
+            self.assertFalse(r1 == r2)
+            self.assertTrue(r1 != r2)
+            def bad1():
+                r1 < r2
+            def bad2():
+                r1 <= r2
+            def bad3():
+                r1 > r2
+            def bad4():
+                r1 >= r2
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad1)
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad2)
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad3)
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad4)
+        finally:
+            dns.rdata._allow_relative_comparisons = saved
+
+    def test_absolute_vs_absolute_compare(self):
+        r1 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www.')
+        r2 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'xxx.')
+        self.assertFalse(r1 == r2)
+        self.assertTrue(r1 != r2)
+        self.assertTrue(r1 < r2)
+        self.assertTrue(r1 <= r2)
+        self.assertFalse(r1 > r2)
+        self.assertFalse(r1 >= r2)
 
+    def test_relative_vs_relative_compare_unstrict(self):
+        try:
+            saved = dns.rdata._allow_relative_comparisons
+            dns.rdata._allow_relative_comparisons = True
+            r1 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www')
+            r2 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'xxx')
+            self.assertFalse(r1 == r2)
+            self.assertTrue(r1 != r2)
+            self.assertTrue(r1 < r2)
+            self.assertTrue(r1 <= r2)
+            self.assertFalse(r1 > r2)
+            self.assertFalse(r1 >= r2)
+        finally:
+            dns.rdata._allow_relative_comparisons = saved
+
+    def test_relative_vs_relative_compare_strict(self):
+        try:
+            saved = dns.rdata._allow_relative_comparisons
+            dns.rdata._allow_relative_comparisons = False
+            r1 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'www')
+            r2 = dns.rdata.from_text(dns.rdataclass.IN, dns.rdatatype.NS, 'xxx')
+            self.assertFalse(r1 == r2)
+            self.assertTrue(r1 != r2)
+            def bad1():
+                r1 < r2
+            def bad2():
+                r1 <= r2
+            def bad3():
+                r1 > r2
+            def bad4():
+                r1 >= r2
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad1)
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad2)
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad3)
+            self.assertRaises(dns.rdata.NoRelativeRdataOrdering, bad4)
+        finally:
+            dns.rdata._allow_relative_comparisons = saved
 
 class UtilTestCase(unittest.TestCase):
 
diff --git a/tests/test_zonedigest.py b/tests/test_zonedigest.py
index f98e5f7..d94be24 100644
--- a/tests/test_zonedigest.py
+++ b/tests/test_zonedigest.py
@@ -176,3 +176,18 @@ class ZoneDigestTestCase(unittest.TestCase):
         with self.assertRaises(dns.exception.SyntaxError):
             dns.rdata.from_text('IN', 'ZONEMD', '100 1 0 ' + self.sha384_hash)
 
+    sorting_zone = textwrap.dedent('''
+    @  86400  IN  SOA     ns1 admin 2018031900 (
+                                     1800 900 604800 86400 )
+              86400  IN  NS      ns1
+              86400  IN  NS      ns2
+              86400  IN  RP      n1.example. a.
+              86400  IN  RP      n1. b.
+    ''')
+    
+    def test_relative_zone_sorting(self):
+        z1 = dns.zone.from_text(self.sorting_zone, 'example.', relativize=True)
+        z2 = dns.zone.from_text(self.sorting_zone, 'example.', relativize=False)
+        zmd1 = z1.compute_digest(dns.zone.DigestHashAlgorithm.SHA384)
+        zmd2 = z2.compute_digest(dns.zone.DigestHashAlgorithm.SHA384)
+        self.assertEqual(zmd1, zmd2)