Separately send large mDNS responses to comply with RFC 6762 by gjbadros · Pull Request #248 · python-zeroconf/python-zeroconf

gjbadros · 2020-05-13T18:44:52Z

This fixes issue #245

Split up large multi-response packets into separate packets instead of relying on IP Fragmentation. IP Fragmentation of mDNS packets causes ChromeCast Audios to crash their mDNS responder processes and RFC 6762
(https://tools.ietf.org/html/rfc6762) section 17 states some
requirements for Multicast DNS Message Size, and the fourth paragraph reads:

"A Multicast DNS packet larger than the interface MTU, which is sent
using fragments, MUST NOT contain more than one resource record."

This change makes this implementation conform with this MUST NOT clause.

… responses won't fit. This is required by section 17 of RFC6762. Make packet() into packets() and have it return a list of the packets that need to be sent to each of the interfaces.

…g) messages.

…the question in the initial packet) and fixed the tests. In particular, the lots_of_names test's behaviour is explicitly supposed to change because the old behaviour was non-conforming.

coveralls · 2020-05-23T01:04:47Z

Coverage decreased (-0.1%) to 92.705% when pulling 6d2a7a1 on gjbadros:master into 445d7f5 on jstasiak:master.

…more and test coverage went down.

gjbadros · 2020-05-23T01:33:21Z

The "X" is now just a very minor code coverage reduction and the missed code coverage is related to the change.

@jstasiak, what are your thoughts? It's pretty clear that the old implementation did not handle extra long packets with lots of responses, and I believe I've done a correct implementation of sending them out as multiple packets. I'm certain this fixes the ChromeCast Audios failing on my network.

Thanks!

…y test log_warning_once from test sutie.

jstasiak

Thank you @gjbadros, it's a solid piece of work! I'll have another look tomorrow, but I think after my comments are addressed it's ready to be merged. I'd not worry about the logging method not being called and lowering the coverage slightly – it's just a number.

jstasiak · 2020-05-24T20:29:18Z



-class DNSOutgoing:
+class DNSOutgoing(QuietLogger):


I don't remember the details about QuietLogger precisely, but its methods are static so let's not inherit from it (I know some classes already do that – I'll remove it).

Ok, I changed mine back but didn't change the others to avoid a merge conflict since it sounds like you're planning to fix those.

jstasiak · 2020-05-24T20:30:45Z

+    def write_record(self, record: DNSRecord, now: float, allow_long: bool = False) -> bool:
        """Writes a record (answer, authoritative answer, additional) to
-        the packet"""
+        the packet.  Returns True on success. """


Can you mention the failure scenarios in this docstring as well?

jstasiak · 2020-05-24T20:31:47Z

+        Generally, you want to use packets() in case the response
+        does not fit in a single packet, but this exists for
+        backward compatibility."""
+        p = self.packets()


I think it's fine to just call the variable packets, not much saved by shortening it to just p and it can help avoiding some confusion.

Ok, I changed it; I don't like local variable names shadowing class instances/functions, but I get that other people don't like short variable names. My philosophy is that a variable names's length should be proportional to its scope and in this case the scope is tiny so I was comfortable with a single-letter variable name.

jstasiak · 2020-05-24T20:32:38Z

+            return b''
+
+    def packets(self) -> List[bytes]:
+        """Returns a list of strings containing the packets' bytes


"bytestrings" instead of "strings"? "strings" implies text.

Sure. I think I was just copying the former docstring language, but this is a good idea to be clear about.

jstasiak · 2020-05-24T20:34:41Z

-            self.insert_short(0, len(self.answers) - overrun_answers)
-            self.insert_short(0, len(self.questions))
+                questions_written += 1
+            allow_long = True  # at most one answer is allowed to be a long packet


Is it at most one answer per packet or per group of packets?

Added docstring -- multiple answers per packet are fine. It's just the case of having so many answers that it overflows the smaller 1470ish MTU of ethernet -- in that scenario, the application layer MUST send separate application-level responses rather than rely on IP fragmentation to break up a bigger packet. The only exception is when a single answer does not fit inside the smaller MTU in which case it's allowed to be bigger. I suspect that scenario will still crash ChromeCast Audios, but that situation is exceedingly rare, whereas many responses that exceed 1470 bytes happens within a large but not crazy set of devices.

jstasiak · 2020-05-24T20:39:32Z

        # wait until the browse request packet has maxed out in size
        sleep_count = 0
-        while sleep_count < 100 and longest_packet_len < r._MAX_MSG_ABSOLUTE - 100:
+        while sleep_count < 100 and longest_packet_len < (5 * r._MAX_MSG_TYPICAL) - 100:


Does 5 have some specific meaning in this context?

No, I changed it back since it's really immaterial now, and I explain why it doesn't matter in a comment.

…e bit more documentation, a variable rename, and a better explanation of a test.

jstasiak

LGTM! I'd like to merge it after #258 is merged and released.

gjbadros · 2020-05-25T22:44:01Z

Yep understood. I'm glad that issue isn't comingled with this change too. Thanks!

…

On Mon, May 25, 2020, 2:55 PM Jakub Stasiak ***@***.***> wrote: ***@***.**** approved this pull request. LGTM! I'd like to merge it after #258 <#258> is merged and released. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#248 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALOHTKTTS6244XH7IZHSJ3RTLSMVANCNFSM4NAAT7KQ> .

- DNSOutgoing.packet only returned a partial message when the DNSOutgoing contents exceeded _MAX_MSG_ABSOLUTE or _MAX_MSG_TYPICAL This was a legacy function that was replaced with .packets() which always returns a complete payload in python-zeroconf#248 As packet() should not be used since it will end up missing data, it has been removed

- DNSOutgoing.packet only returned a partial message when the DNSOutgoing contents exceeded _MAX_MSG_ABSOLUTE or _MAX_MSG_TYPICAL This was a legacy function that was replaced with .packets() which always returns a complete payload in #248 As packet() should not be used since it will end up missing data, it has been removed

gjbadros added 9 commits May 8, 2020 19:11

Send multiple packets in a DNSOutgoing response when multiple smaller…

e3268d2

… responses won't fit. This is required by section 17 of RFC6762. Make packet() into packets() and have it return a list of the packets that need to be sent to each of the interfaces.

Fixed a type declaration, and made new log messages debug (not warnin…

6a7ae11

…g) messages.

Add back packet() for backward compatability.

311da47

Protect against empty packets()

0524706

Fixed one bug in the implementation (make sure we at least write out …

0231086

…the question in the initial packet) and fixed the tests. In particular, the lots_of_names test's behaviour is explicitly supposed to change because the old behaviour was non-conforming.

Merge branch 'master' into master

e2b02e3

Change formatting to make flake8 happy

19e88f7

:xMerge branch 'master' of https://github.com/gjbadros/python-zeroconf

e322d29

Fix test.py to comply with flake8 warnings

f737898

gjbadros added 2 commits May 22, 2020 18:07

Fix test.py to comply with flake8 warnings

fc1ac81

Added a new test to call log_warning_once since that wasn't used any …

08101fb

…more and test coverage went down.

Warn about extra-large packet upon creation, too, in order to properl…

3db6a1d

…y test log_warning_once from test sutie.

jstasiak requested changes May 24, 2020

View reviewed changes

jstasiak mentioned this pull request May 24, 2020

Enable Mac OS CPython builds in CI #194

Closed

gjbadros added 2 commits May 24, 2020 16:09

Made changes that are responsive to Jakub's review, including a littl…

a444619

…e bit more documentation, a variable rename, and a better explanation of a test.

Merge branch 'master' into master

6d2a7a1

jstasiak approved these changes May 25, 2020

View reviewed changes

jstasiak merged commit 87a0fe2 into python-zeroconf:master May 26, 2020

jstasiak mentioned this pull request May 27, 2020

mdns-repeater causes ChromeCast Audio devices to cease broadcasting mDNS responses when interface MTU exceeded #245

Closed

kennylevinsen mentioned this pull request Jun 22, 2020

mdns-repeater causes ChromeCast Audio devices to cease broadcasting mDNS responses when interface MTU exceeded geekman/mdns-repeater#6

Closed

bdraco mentioned this pull request Jun 13, 2021

Remove DNSOutgoing.packet backwards compatibility #569

Merged

Conversation

gjbadros commented May 13, 2020

Uh oh!

coveralls commented May 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gjbadros commented May 23, 2020

Uh oh!

jstasiak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jstasiak left a comment

Choose a reason for hiding this comment

Uh oh!

gjbadros commented May 25, 2020 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented May 23, 2020 •

edited

Loading