Repository: postlund/pyatv
Pull request #927 (2021-01-12 23:25:35)

Use UTF-8 for DNS names, and (attempt) to handle dots in names
Unfortunately I was unable to add emoji to my Apple TV's name, but if Apple ever fixes that, this should handle that too.

codecov[bot]: # [Codecov](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=h1) Report
> Merging [#927](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=desc) (16234fe) into [master](https://codecov.io/gh/postlund/pyatv/commit/f0f7eaf64c824d40ba72ef727ffb37ea87979f25?el=desc) (f0f7eaf) will **increase** coverage by `0.06%`.
> The diff coverage is `98.18%`.

[![Impacted file tree graph](https://codecov.io/gh/postlund/pyatv/pull/927/graphs/tree.svg?width=650&height=150&src=pr&token=wb6azJjRSY)](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
+ Coverage   95.71%   95.77%   +0.06%     
==========================================
  Files          40       38       -2     
  Lines        3755     3338     -417     
==========================================
- Hits         3594     3197     -397     
+ Misses        161      141      -20     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `95.77% <98.18%> (?)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pyatv/support/dns.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvc3VwcG9ydC9kbnMucHk=) | `95.39% <98.03%> (+1.57%)` | :arrow_up: |
| [pyatv/support/mdns.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvc3VwcG9ydC9tZG5zLnB5) | `87.94% <100.00%> (+0.05%)` | :arrow_up: |
| [pyatv/airplay/server.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvYWlycGxheS9zZXJ2ZXIucHk=) | `90.32% <0.00%> (-0.59%)` | :arrow_down: |
| [pyatv/mrp/connection.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvbXJwL2Nvbm5lY3Rpb24ucHk=) | `81.91% <0.00%> (-0.20%)` | :arrow_down: |
| [pyatv/dmap/\_\_init\_\_.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvZG1hcC9fX2luaXRfXy5weQ==) | `97.00% <0.00%> (-0.13%)` | :arrow_down: |
| [pyatv/mrp/pairing.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvbXJwL3BhaXJpbmcucHk=) | `97.56% <0.00%> (-0.12%)` | :arrow_down: |
| [pyatv/dmap/pairing.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvZG1hcC9wYWlyaW5nLnB5) | `96.55% <0.00%> (-0.08%)` | :arrow_down: |
| [pyatv/mrp/srp.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvbXJwL3NycC5weQ==) | `96.52% <0.00%> (-0.03%)` | :arrow_down: |
| [pyatv/dmap/daap.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvZG1hcC9kYWFwLnB5) | `97.16% <0.00%> (-0.03%)` | :arrow_down: |
| [pyatv/interface.py](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree#diff-cHlhdHYvaW50ZXJmYWNlLnB5) | `99.25% <0.00%> (-0.01%)` | :arrow_down: |
| ... and [7 more](https://codecov.io/gh/postlund/pyatv/pull/927/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=footer). Last update [f0f7eaf...16234fe](https://codecov.io/gh/postlund/pyatv/pull/927?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

paxswill: My old nemesis...

Just thought of a case this won't handle: if someone deliberately sets their Apple TV's name to include "_foo._tcp", meaning full names like `_foo._tcp._airplay._tcp.local` won't work (and fixing that would be a much larger diff).
postlund: Tested on my end and my Apple TV 3 is now found again, so it seems to work 👍 Looks good otherwise, so I'll merge once update is complete. The case you mention sound a lot like a very obscure corner case, so I suggest that we skip it. 

Patch:
diff --git a/pyatv/support/dns.py b/pyatv/support/dns.py
index a256fab2..44e72829 100644
--- a/pyatv/support/dns.py
+++ b/pyatv/support/dns.py
@@ -1,10 +1,12 @@
 """Processing functions for raw DNS messages."""
+import collections.abc
 import enum
 import io
 import logging
 import struct
 import typing
 from ipaddress import IPv4Address
+import unicodedata
 
 from pyatv.support.collections import CaseInsensitiveDict
 
@@ -21,35 +23,112 @@ def unpack_stream(fmt: str, buffer: typing.BinaryIO) -> typing.Tuple:
     return struct.unpack(fmt, buffer.read(size))
 
 
-def qname_encode(name: str) -> bytes:
-    """Encode QNAME without using name compression."""
+class ServiceInstanceName(typing.NamedTuple):
+    """Represents either a service or service instance name in the DNS.
+
+    The special thing this class can do is (attempt) to handle periods in the instance
+    name correctly.
+    """
+
+    instance: typing.Optional[str]
+    service: str
+    domain: str = "local"
+
+    def __str__(self):
+        """Join all components together with periods."""
+        return ".".join(typing.cast(typing.Iterable[str], filter(None, self)))
+
+    @classmethod
+    def split_name(cls, name: str) -> "ServiceInstanceName":
+        """Split a name into instance (optional), service, and domain parts.
+
+        If the name given isn't a service name or service instance name, this method
+        raise a `ValueError`.
+        """
+        labels = name.split(".")
+        if len(labels) < 2:
+            raise ValueError("There must be at least three labels in a service name")
+        for index in range(len(labels) - 1):
+            label, next_label = labels[index : index + 2]
+            if label.startswith("_") and next_label.lower() in ("_tcp", "_udp"):
+                return cls(
+                    ".".join(labels[0:index]) or None,
+                    label + "." + next_label,
+                    ".".join(labels[index + 2 :]),
+                )
+        raise ValueError(
+            f"'{name}' is not a service domain, nor a service instance name"
+        )
+
+    @property
+    def ptr_name(self):
+        """Return just the service name, like the name for a PTR record."""
+        return ".".join((self.service, self.domain))
+
+
+def qname_encode(name: typing.Union[str, typing.Sequence[str]]) -> bytes:
+    """Encode QNAME without using name compression.
+
+    Labels (each component of a domain name) are encoded using UTF-8, as that is what
+    the Apple TV has been observed to use for all domain names.
+
+    This function can take either a single string, with each label separated by dots, or
+    a sequence of strings, and each element of the sequence is treated as a single
+    label. A null (empty) label is added for the root domain if it is not already
+    present for both types of arguments.
+    """
     encoded = bytearray()
-    labels = name.split(".")
-    # Ensure there's an empty label for the root domain
-    if labels[-1] != "":
+    labels: typing.List[str]
+    if isinstance(name, collections.abc.Sequence) and not isinstance(name, str):
+        # Copy the sequence so we can make changes to it
+        labels = list(name) or []
+    else:
+        # Try to parse it as a service instance name, so we can handle the instance
+        # label having dots in it.
+        try:
+            srv_name = ServiceInstanceName.split_name(name)
+        except ValueError:
+            labels = typing.cast(str, name).split(".")
+        else:
+            labels = []
+            if srv_name.instance:
+                labels.append(srv_name.instance)
+            # the ptr_name just has the instance name dropped off
+            labels.extend(srv_name.ptr_name.split("."))
+    # Ensure there's always an empty label for the root domain
+    if not labels or labels[-1] != "":
         labels.append("")
-    for label in labels:
-        encoded_label = label.encode("idna")
+    # DNS-SD uses UTF-8 for names, not IDNA. Apple extends this to basically all places
+    # where names are used (except for A/AAAA records, which are transliterated!).
+    encoding = "utf-8"
+    # Normalize all labels using NFC, as specified in RFC 6763, section 4.1.3
+    normalized_labels = (unicodedata.normalize("NFC", label) for label in labels)
+    for label in normalized_labels:
+        encoded_label = label.encode(encoding)
         encoded_length = len(encoded_label)
-        # Length of the encoded label, in bytes, but a maximum of 63
-        # The maximum is 63 as the upper two bits are used as a flag for name
-        # compression.
-        encoded.append(min(encoded_length, 63))
-        if encoded_length == 0:
-            # If we've reached an empty label, assume this is the last component.
-            # Empty labels (two periods right after each other) isn't legal anyways.
-            break
-        if encoded_length > 63:
+        # When truncating the label, we can't just stop at 63 bytes as that might be
+        # splitting a multi-byte Unicode codepoint.
+        truncated = False
+        while encoded_length > 63:
+            truncated = True
+            truncated_label = encoded_label.decode(encoding)[:-1]
+            encoded_label = truncated_label.encode(encoding)
+            encoded_length = len(encoded_label)
+        if truncated:
             _LOGGER.warning(
                 (
-                    "A label (%s) isn being truncated (to %s) in the DNS name '%s' "
+                    "A label (%s) is being truncated (to %s) in the DNS name '%s' "
                     "as it is over 63 bytes long."
                 ),
                 label,
-                label[:64],
+                encoded_label.decode(encoding),
                 name,
             )
-            encoded.extend(encoded_label[:64])
+        encoded.append(encoded_length)
+        if encoded_length == 0:
+            # If we've reached an empty label, assume this is the last component.
+            # Empty labels (two periods right after each other) aren't legal anyways.
+            break
         else:
             encoded.extend(encoded_label)
     return encoded
@@ -70,10 +149,14 @@ def parse_domain_name(buffer: typing.BinaryIO) -> str:
     """Unpack a domain name, handling any name compression encountered.
 
     Basically, each component of a domain name (called a "label") is prefixed with a
-    length followed by the IDNA encoded label. The final component has a zero length
+    length followed by the encoded label. The final component has a zero length
     with a null label for the DNS root. The tricky part is that labels are limited to 63
     bytes, and the upper two bits of the length are used as a flag for "name
-    compression". For full details, see RFC1035, sections 3.1 and 4.1.4.
+    compression". For full details, see RFC 1035, sections 3.1 and 4.1.4.
+
+    If labels start with the "ASCII Compatible Encoding" prefix ("xn--"), they are
+    decoded with IDNA. Otherwise each label is decoded as UTF-8, as that is what is used
+    for DNS-SD and Apple doesn't seem to use IDNA anywhere in their mDNS/DNS-SD stack.
 
     This is distinct from "character-string" encoding; use `parse_string` for that.
     """
@@ -102,10 +185,14 @@ def parse_domain_name(buffer: typing.BinaryIO) -> str:
             buffer.seek(new_offset)
         elif length_flags == 0:
             label = buffer.read(length)
-            labels.append(label)
+            if label[:4] == b"xn--":
+                decoded_label = label.decode("idna")
+            else:
+                decoded_label = label.decode("utf-8")
+            labels.append(decoded_label)
     if compression_offset is not None:
         buffer.seek(compression_offset)
-    return ".".join(label.decode("idna") for label in labels)
+    return ".".join(labels)
 
 
 def parse_txt_dict(buffer: typing.BinaryIO, length: int) -> CaseInsensitiveDict[bytes]:
diff --git a/pyatv/support/mdns.py b/pyatv/support/mdns.py
index 66dadf17..8a42d567 100644
--- a/pyatv/support/mdns.py
+++ b/pyatv/support/mdns.py
@@ -11,7 +11,13 @@ from zeroconf import Zeroconf, ServiceInfo
 
 from pyatv.support import log_binary, net
 from pyatv.support.collections import CaseInsensitiveDict
-from pyatv.support.dns import DnsMessage, DnsQuestion, DnsResource, QueryType
+from pyatv.support.dns import (
+    DnsMessage,
+    DnsQuestion,
+    DnsResource,
+    QueryType,
+    ServiceInstanceName,
+)
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -91,9 +97,9 @@ def parse_services(message: DnsMessage) -> typing.List[Service]:
 
     # Build services
     for service, device in table.items():
-        service_name, _, service_type = service.partition(".")
-
-        if not service_type.endswith("_tcp.local"):
+        try:
+            service_name = ServiceInstanceName.split_name(service)
+        except ValueError:
             continue
 
         port = (QueryType.SRV in device and device[QueryType.SRV].rd["port"]) or 0
@@ -106,8 +112,8 @@ def parse_services(message: DnsMessage) -> typing.List[Service]:
         address = IPv4Address(target_record.rd) if target_record else None
 
         results[service] = Service(
-            service_type,
-            service_name,
+            service_name.ptr_name,
+            typing.cast(str, service_name.instance),
             address,
             port,
             _decode_properties(properties),


Test patch:
diff --git a/tests/support/test_dns.py b/tests/support/test_dns.py
index 213578b9..a44059d0 100644
--- a/tests/support/test_dns.py
+++ b/tests/support/test_dns.py
@@ -7,12 +7,81 @@ import pytest
 from pyatv.support import dns
 
 
+@pytest.mark.parametrize(
+    "name,expected",
+    (
+        ("_http._tcp.local", (None, "_http._tcp", "local")),
+        ("foo._http._tcp.local", ("foo", "_http._tcp", "local")),
+        ("foo.bar._http._tcp.local", ("foo.bar", "_http._tcp", "local")),
+    ),
+    ids=("ptr", "no_dot", "with_dot"),
+)
+def test_happy_service_instance_names(name, expected):
+    assert dns.ServiceInstanceName.split_name(name) == expected
+
+
+@pytest.mark.parametrize(
+    "name",
+    (
+        "_http.local",
+        "._tcp.local",
+        "_http.foo._tcp.local",
+        "_tcp._http.local",
+    ),
+    ids=("no_proto", "no_service", "split", "reversed"),
+)
+def test_sad_service_instance_names(name):
+    with pytest.raises(ValueError):
+        dns.ServiceInstanceName.split_name(name)
+
+
 # mapping is test_id: tuple(name, expected_raw)
 encode_domain_names = {
     "root": (".", b"\x00"),
-    "example.com": (".", b"\x00"),
-    # Taken straight from Wikipedia's IDNA page
-    "idna": ("Bücher.example", b"\x0Dxn--bcher-kva\x07example\x00"),
+    "empty": ("", b"\x00"),
+    "example.com": ("example.com", b"\x07example\x03com\x00"),
+    "example.com_list": (["example", "com"], b"\x07example\x03com\x00"),
+    "unicode": ("Bücher.example", b"\x07B\xc3\xbccher\x07example\x00"),
+    "dotted_instance": (
+        "Dot.Within._http._tcp.example.local",
+        b"\x0aDot.Within\x05_http\x04_tcp\x07example\x05local\x00",
+    ),
+    "dotted_instance_list": (
+        ["Dot.Within", "_http", "_tcp", "example", "local"],
+        b"\x0aDot.Within\x05_http\x04_tcp\x07example\x05local\x00",
+    ),
+    "truncated_ascii": (
+        (
+            "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
+            "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
+            ".test"
+        ),
+        (
+            b"\x3fabcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijk"
+            b"\x04test"
+            b"\x00"
+        ),
+    ),
+    "truncated_unicode": (
+        (
+            # The 'a' is at the beginning to force the codepoints to be split at 63
+            # bytes. The next line is also at the right length to be below 88 characters
+            # even if each kana is counted as a double-width character. Additionally,
+            # this sequence is NF*D* normalized, not NFC (which is what is used for
+            # Net-Unicode).
+            "aがあいうえおかきくけこさしすせそたちつてとなにぬねのはひふへほまみむめも"
+            ".test"
+        ),
+        (
+            b"\x3d"
+            b"a\xe3\x81\x8c\xe3\x81\x82\xe3\x81\x84\xe3\x81\x86\xe3\x81\x88\xe3\x81\x8a"
+            b"\xe3\x81\x8b\xe3\x81\x8d\xe3\x81\x8f\xe3\x81\x91\xe3\x81\x93\xe3\x81\x95"
+            b"\xe3\x81\x97\xe3\x81\x99\xe3\x81\x9b\xe3\x81\x9d\xe3\x81\x9f\xe3\x81\xa1"
+            b"\xe3\x81\xa4\xe3\x81\xa6"
+            b"\x04test"
+            b"\x00"
+        ),
+    ),
 }
 
 
@@ -38,8 +107,27 @@ decode_domain_names = {
         "foo.label.test",
         -2,
     ),
-    # Again, take straight from the Internationalized Domain name Wikipedia page
+    # Taken straight from the Internationalized Domain name Wikipedia page
     "idna": (b"\x0Dxn--bcher-kva\x07example\x00", 0, "bücher.example", None),
+    # Taken from issue #919. Apple puts a non-breaking space between "Apple" and "TV".
+    "nbsp": (
+        b"\x10Apple\xc2\xa0TV (4167)\x05local\x00",
+        0,
+        "Apple\xa0TV (4167).local",
+        None,
+    ),
+    # This is a doozy of a test case; it's covering a couple different areas of Unicode,
+    # as well as exercising that DNS-SD allows dots in instance names.
+    "unicode": (
+        (
+            b"\x1d\xe5\xb1\x85\xe9\x96\x93 Apple\xc2\xa0TV. En Espa\xc3\xb1ol"
+            b"\x05local"
+            b"\x00"
+        ),
+        0,
+        "居間 Apple TV. En Español.local",
+        None,
+    ),
 }