[multicast_dns] Rewrite _readFQDN to avoid recursion

[andrewkolos]

Speculatively fixes flutter/flutter#155433. Read one of my later comments on that thread for my current understanding of this issue. In summary, this PR rewrites _readFQDN to avoid recursion.

For reviewers: For a simplified diff, see the first commit, change. This commit avoids formatting make sure the diff only highlights changes to logic rather than formatting.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• x ] I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[bkonyi]

This change LGTM overall, but I'm wondering if it's just going to cause us to get into an infinite loop rather than a stack overflow, which is going to be much harder to identify.

Since we can't catch the stack overflow error, I wonder if it's worth adding a top-level debug property here that contains the packet that's currently being processed. Then, the tool can add use a timer to determine if we're spending too much time processing the MDNS response to dump a base64 encoded packet to the terminal along with a message asking users to file a bug and share the packet contents with us (assuming they're comfortable sharing it since it could possibly contain PII in the domain). WDYT?

[andrewkolos]

I verified that this is actually detecting a cycle using the debugger. We could refactor decodeMDnsResponse to return some sort of ADT that can indicate a reason for failure instead of always returning null. Then, tests could make sure that null is being returned by the expected reason. However, I want to make sure you are OK with this cycle-detection approach before I invest in that effort.

[andrewkolos]

I think an infinite loop can be avoided with some cycle detection, so I've pushed that change. I do like your idea though, so let me know if you have a strong preference here. One problem I foresee with it is that users would have to decode the packet themselves if want to know what exactly what they are reporting.

[bkonyi]

Is checking for previously visited offsets sufficient with an iterative approach like this? I could be wrong, but I think that it's possible and valid to visit offsets multiple times when decoding different parts. If that's the case, reverting to using a recursive approach where we track the previously visited offsets that are currently on the stack would probably be more robust and avoid false positives.

I do think it's going to be useful to provide users a way to get access to the packet data for reporting purposes. I think as long as we provide notice that there might be PII in this data and tell the user to file an issue with or without the data, we can always have them reach out to us privately with the data.

[andrewkolos]

Is checking for previously visited offsets sufficient with an iterative approach like this? I could be wrong, but I think that it's possible and valid to visit offsets multiple times when decoding different parts.

I spent some more time (probably too much 🙃) re-scrutinizing section 4.1.4, and I think that such a record would be invalid. Recall that _readFQDN is only reading one domain name. Then, consider:

The compression scheme allows a domain name in a message to be
represented as either:

• a sequence of labels ending in a zero octet

• a pointer

• a sequence of labels ending with a pointer

My interpretation of this is that, while pointers might be allowed form a chain (e.g. a pointer points to either 1) another pointer or 2) a sequence of labels ending with another pointer), a single NAME section cannot contain more than one pointer. For example, I don't believe foo + <pointer to earlier offset representing bar> + <another pointer to the same bar> is valid.

While looking for other interpretations of this, I found some not particularly useful StackExchange threads ([1], [2]). However, I did find this interesting bit from a 2022 article written by a Tony Finch that appears to have a long career revolving around DNS stuff:

My favourite slapstick bug in DNS software is due to DNS name compression. If the programmer is not careful enough, their DNS message parsing code can get into an infinite loop when trying to decode a compressed name.

Many, many DNS experts have made this mistake.

The neatest way to avoid the bug is to keep an upper limit for compression offsets, which is initialized with the offset of the start of the name. When the decoder encounters a compression pointer, it must be less than the limit or the message is malformed. If it is OK, then both the decoder’s position and the compression upper limit are set to the pointer’s target.

This appears to just be a more clever way of what I have implemented here (basically you are just making sure you only follow pointers to offsets that are before whatever you have already visited). Well, actually it reveals another issue with _readFQDN, but I'll leave that as a footnote¹.

In conclusion, I am decently confident that this change is effective in preventing infinite loops without reducing correctness. As I understand things, however, I think we are just dropping invalid packets on the floor. Maybe we should provide a way to listen for these and log them in the flutter tool? This is a separate issue, though.

Footnotes

1. The RFC states "In this scheme, an entire domain name or a list of labels at the end of a domain name is replaced with a pointer to a prior occurance [sic.] of the same name." Considering this, Finch's method is not only more efficient than mine, but also more correct, since the RFC disallows pointers to a later offset. ↩

[bkonyi]

Thanks for digging into this! I was staring at the spec for longer than I'd like to admit too... :)

This LGTM, but I definitely think we should notify users of invalid packets in a future PR.

[auto-submit]

autosubmit label was removed for flutter/packages/8390, because - The status or check suite Mac_x64 build_all_packages stable has failed. Please fix the issues identified (or deflake) before re-applying this label.