Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[multicast_dns] Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency - #79772 #6700

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

biagiopietro
Copy link

@biagiopietro biagiopietro commented May 9, 2024

I encountered this package long time ago (see linked issue below) and there were cases where it wasn't working.
After 3 years (yeah it's more time than expected) I managed to find the time to dust off wireshark and have look again.

Preamble

Considering the following setup

image

Where Raspberry pi runs a mDNS service using the following go:

main.go

package main

import (
	"fmt"
	"github.com/hashicorp/mdns"
	"os"
  "net/http"
)

func health(w http.ResponseWriter, r *http.Request) {
  fmt.Fprintf(w, "ok")
}

func main() {

	hostname, err := os.Hostname()

	if err != nil {
		panic(fmt.Sprintf("Error getting current hostname, description: %s", err.Error()))
	}

	info := []string{"mDNS get server"}

	service, err := mdns.NewMDNSService(hostname, "_test._tcp", "", "", 8080, nil, info)

	if err != nil {
		panic(fmt.Sprintf("Error while exporting the service, description: %s", err.Error()))
	}

	server, err := mdns.NewServer(&mdns.Config{Zone: service})

	if err != nil {
		panic(fmt.Sprintf("Error while setting the discover server up, description: %s", err.Error()))
	}

	defer server.Shutdown()
	
	http.HandleFunc("/", health)
	http.ListenAndServe(":8081",nil)
}

Considering the following client (which I got from here):

client.dart

// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// Example script to illustrate how to use the mdns package to discover the port
// of a Dart observatory over mDNS.

// ignore_for_file: avoid_print

import 'package:multicast_dns/multicast_dns.dart';

Future<void> main() async {
  // Parse the command line arguments.

  const String name = '_test._tcp.local';
  final MDnsClient client = MDnsClient();
  // Start the client with default options.
  await client.start();

  // Get the PTR record for the service.
  await for (final PtrResourceRecord ptr in client
      .lookup<PtrResourceRecord>(ResourceRecordQuery.serverPointer(name))) {
    // Use the domainName from the PTR record to get the SRV record,
    // which will have the port and local hostname.
    // Note that duplicate messages may come through, especially if any
    // other mDNS queries are running elsewhere on the machine.
    await for (final SrvResourceRecord srv in client.lookup<SrvResourceRecord>(
        ResourceRecordQuery.service(ptr.domainName))) {
      // Domain name will be something like "io.flutter.example@some-iphone.local._dartobservatory._tcp.local"
      final String bundleId =
          ptr.domainName; //.substring(0, ptr.domainName.indexOf('@'));
      print('Dart observatory instance found at '
          '${srv.target}:${srv.port} for "$bundleId".');
    }
  }
  client.stop();

  print('Done.');
}

What happens

When running the client script (dart run client.dart so with the latest package of multicast_dns package) as is, a list of sockets is created which are bind to port 5353 and IPs:

  • 0.0.0.0;
  • 127.0.0.1;
  • 192.168.2.16;
  • 172.17.0.1;

a list of interfaces (see list below) are joined to the multicast socket which is bound to 0.0.0.0:5353:

  • lo (with address 127.0.0.1);
  • wlan0 (with address 192.168.2.16);
  • docker0 (with address 172.17.0.1).

and eventually when lookup function is being called QMqueries are being sent from ALL the sockets in the list; which means that for 0.0.0.0 the IP address chosen by the operating system will depend on various factors such as the routing table, the default network interface, and the specific configuration of the network interfaces on the machine. It could be sent from any of the IP addresses associated with the machine's network interfaces, including IP addresses assigned to physical network adapters or virtual interfaces.

Using Wireshark, I can see that 2 QM packets are being sent and I can see that mDNS service is responding to the client with proper packet but it seems that the socket opened at 0.0.0.0:5353 is not reading them at all even though the socket is still open.

Source	        Destination	Protocol	Length	Info
192.168.2.16	224.0.0.251	MDNS		    76	Standard query 0x0000 PTR _test._tcp.local, "QM" question
192.168.2.16	224.0.0.251	MDNS		    76	Standard query 0x0000 PTR _test._tcp.local, "QM" question
192.168.2.7	192.168.2.16	MDNS		   180	Standard query response 0x0000 PTR mdnsserv._test._tcp.local SRV 10 1 8080 mdnsserv A 127.0.1.1 TXT
192.168.2.7	192.168.2.16	MDNS		   180	Standard query response 0x0000 PTR mdnsserv._test._tcp.local SRV 10 1 8080 mdnsserv A 127.0.1.1 TXT

First approach (not sure if it's RFC 6762 friendly)

I had the "feeling" that sending QM packets to 0.0.0.0:5353 and other interfaces on the same port would generate some sort of unexpected behavior due to the nature of 0.0.0.0 which IP selections depends on multiple factors.

Therefore I tried initially to change the incoming socket (the one bound to 0.0.0.0) from:

final RawDatagramSocket incoming = await _rawDatagramSocketFactory(
      listenAddress.address,
      selectedMDnsPort,
      reuseAddress: true,
      reusePort: true,
      ttl: 255,
    );

to

final RawDatagramSocket incoming = await _rawDatagramSocketFactory(
      listenAddress.address,
      0,
      reuseAddress: true,
      reusePort: true,
      ttl: 255,
    );

which essentially delegates to OS to choose a random port (instead of forcing 5353).

In this case the client managed to process correctly all the packages for discovering the mDNS service, indeed in Wireshark I could see:

Source	        Destination	Protocol	Length	Info
192.168.2.16	224.0.0.251	MDNS		    76	Standard query 0x0000 PTR _test._tcp.local, "QM" question
192.168.2.16	224.0.0.251	MDNS		    76	Standard query 0x0000 PTR _test._tcp.local, "QM" question
192.168.2.7	192.168.2.16	MDNS		   180	Standard query response 0x0000 PTR mdnsserv._test._tcp.local SRV 10 1 8080 mdnsserv A 127.0.1.1 TXT
192.168.2.7	192.168.2.16	MDNS		   180	Standard query response 0x0000 PTR mdnsserv._test._tcp.local SRV 10 1 8080 mdnsserv A 127.0.1.1 TXT
192.168.2.16	224.0.0.251	MDNS		    85	Standard query 0x0000 SRV mdnsserv._test._tcp.local, "QM" question
192.168.2.16	224.0.0.251	MDNS		    85	Standard query 0x0000 SRV mdnsserv._test._tcp.local, "QM" question
192.168.2.7	192.168.2.16	MDNS		   123	Standard query response 0x0000 SRV 10 1 8080 mdnsserv A 127.0.1.1
192.168.2.7	192.168.2.16	MDNS		   123	Standard query response 0x0000 SRV 10 1 8080 mdnsserv A 127.0.1.1

and on the client I could see the message:

Dart observatory instance found at mdnsserv:8080 for "mdnsserv._test._tcp.local"

⚠️ : Again, I'm not sure if it can be considered a solution because I dunno isRFC 6762 friendly, I checked some packages which implement mDNS clients and I saw some of them doing what I proposed. I would like to hear comments about it.

Second approach (which it's what is presented in this PR)

After trying the first approach I realized that maybe there is no need to open sockets on more interfaces (and therefore send QM messages) it maybe be enough to send and listen only on a socket bound to 0.0.0.0 since, again, listen on ANY IP and send packets from a selected IP address chosen by the OS.

Also in this case the client managed to process correctly all the packages for discovering the mDNS service, indeed in Wireshark I could see:

Source	        Destination	Protocol	Length	Info
192.168.2.16	224.0.0.251	MDNS		    76	Standard query 0x0000 PTR _test._tcp.local, "QM" question
192.168.2.7	192.168.2.16	MDNS		   180	Standard query response 0x0000 PTR mdnsserv._test._tcp.local SRV 10 1 8080 mdnsserv A 127.0.1.1 TXT
192.168.2.16	224.0.0.251	MDNS		    85	Standard query 0x0000 SRV mdnsserv._test._tcp.local, "QM" question
192.168.2.7	192.168.2.16	MDNS		   123	Standard query response 0x0000 SRV 10 1 8080 mdnsserv A 127.0.1.1

and on the client I could see the message:

Dart observatory instance found at mdnsserv:8080 for "mdnsserv._test._tcp.local"

Third approach (It did not work but mentioning for completeness)

The idea here is to don't send QM packets via 0.0.0.0 but just listen on possible response/s since packets would be send via the following IPs and 0.0.0.0 should represent ANY IP.

  • 127.0.0.1;
  • 192.168.2.16;
  • 172.17.0.1.

Fourth approach (It did not work but mentioning for completeness)

Another solution that I tried but unfortunately it did not work, was to put 0.0.0.0 as last item in the socket list so QM packets would be sent according to the following order:

  • 127.0.0.1;
  • 192.168.2.16;
  • 172.17.0.1;
  • 0.0.0.0.
multicast_dns.start() function

  Future<void> start({
    InternetAddress? listenAddress,
    NetworkInterfacesFactory? interfacesFactory,
    int mDnsPort = mDnsPort,
    InternetAddress? mDnsAddress,
  }) async {
    listenAddress ??= InternetAddress.anyIPv4;
    interfacesFactory ??= allInterfacesFactory;

    assert(listenAddress.address == InternetAddress.anyIPv4.address ||
        listenAddress.address == InternetAddress.anyIPv6.address);

    if (_started || _starting) {
      return;
    }
    _starting = true;

    final int selectedMDnsPort = _mDnsPort = mDnsPort;
    _mDnsAddress = mDnsAddress;

    // Listen on all addresses.
    final RawDatagramSocket incoming = await _rawDatagramSocketFactory(
      listenAddress.address,
      selectedMDnsPort,
      reuseAddress: true,
      reusePort: true,
      ttl: 255,
    );

    _mDnsAddress ??= incoming.address.type == InternetAddressType.IPv4
        ? mDnsAddressIPv4
        : mDnsAddressIPv6;

    final List<NetworkInterface> interfaces =
        (await interfacesFactory(listenAddress.type)).toList();

    for (final NetworkInterface interface in interfaces) {
      // Create a socket for sending on each adapter.
      final InternetAddress targetAddress = interface.addresses[0];
      // Join multicast on this interface.
      incoming.joinMulticast(_mDnsAddress!, interface);
    }

    // Can't send to IPv6 any address.
    if (incoming.address != InternetAddress.anyIPv6) {
      _sockets.add(incoming);
    } else {
      _toBeClosed.add(incoming);
    }

    incoming.listen((RawSocketEvent event) => _handleIncoming(event, incoming));
    _started = true;
    _starting = false;
  }

The idea is indeed to let the first 3 IPs to send the QM packets which response should be hopefully captured by the incoming socket before the socket on 0.0.0.0 would send the QM packet too.

Wireshark filter

(ip.src==192.168.2.7 || ip.src==192.168.2.16) && udp.port eq 5353

Related Issue

  • It should resolves issue #79772

Disclaimers

  • I'm not expert in flutter/dart, I pulled the code and I tried to debug it with help of uncle google and print();
  • I don't have a huge expertise in networking but I know how to play a bit with Wireshark, inspect the networks and craft packets.

Pre-launch Checklist

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

@biagiopietro biagiopietro marked this pull request as draft May 9, 2024 08:00
@biagiopietro biagiopietro marked this pull request as ready for review May 9, 2024 11:52
@jmagman jmagman changed the title [multicast_mdns] Optimized Socket Binding: Consolidated to 0.0.0.0 for Simplicity and Efficiency - #79772 [multicast_dns] Optimized Socket Binding: Consolidated to 0.0.0.0 for Simplicity and Efficiency - #79772 May 9, 2024
@jmagman jmagman linked an issue May 9, 2024 that may be closed by this pull request
@jmagman jmagman requested a review from cbracken May 23, 2024 19:27
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay reviewing this.

@vashworth kindly tested this out against the flutter CLI and confirmed it didn't break wireless debugging for flutter run or flutter attach.

This change generally LGTM, though I would like a second opinion from @cbracken who might remember more about why the original code was written that way.

  1. This needs a pubspec version bump and CHANGELOG update. See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version-and-changelog-updates
  2. Can you add a test to https://github.com/flutter/packages/blob/main/packages/multicast_dns/test/client_test.dart?

@cbracken
Copy link
Member

Taking a look!

@cbracken
Copy link
Member

I did a bit of exploratory git blameing to see if I could figure out the history of the code in question in case there's any additional useful info in log comments (there wasn't), but for the record:

The existing code was derived from the (long-dead, now) Dartino code and was added in
a062a28

It's derived from older Dartino code from the Dart SDK. The most recent version of that can be viewed here:
https://github.com/dart-archive/sdk/tree/master/pkg/mdns

The Dartino mdns package was originally added in this commit:
dart-archive/sdk@f0d30ec

Looking at the actual logic in the context of the rest of the code now.

@cbracken
Copy link
Member

cbracken commented May 23, 2024

Thanks for sending the patch.

Thinking about this a bit, I agree that setting up a single socket on 0.0.0.0 should be sufficient for listening on all network interfaces. As you note, for sending, we'll rely on the operating system's routing to determine which network interface is used for sending. That all seems fine.

The current package doesn't really allow for interface-specific tuning/filtering so I don't think we're losing anything here and the code and resource management is significantly simpler.

@sgjesse wrote the original code and while he's moved on to other projects, may have thoughts. :)

The existing tests should cover the majority of the behaviour here, but as @jmagman says, please add a test that verifies the change in behaviour. If you check the existing tests, you can see how to use the rawDatagramSocketFactory and interfacesFactory constructor parameters to hook socket creation and listing network interfaces.

You could probably write a test where the interfacesFactory lists a bunch of interfaces and the rawDatagramSocketFactor:

  1. increments a counter each time it's called
  2. records the targetAddress in a list

then verify that the counter is == 1 and that the only targetAddress passed in was InternetAddress.anyIPv4.address or InternetAddress.anyIPv6.address, whichever you passed in for mDnsAddress in the constructor.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good modulo comments + as @jmagman says, please add a test (suggestion above).

packages/multicast_dns/CHANGELOG.md Outdated Show resolved Hide resolved
packages/multicast_dns/CHANGELOG.md Outdated Show resolved Hide resolved
reusePort: true,
ttl: 255,
);
_sockets.add(socket);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we're going to rip out the per-interface sockets, then the List<RawDatagramSocket> _sockets list should be replaced with RawDatagramSocket _incomingIPv4 since the socket on which we're listening is the only remaining use for that sockets list. For consistency, I'd be tempted to do the same for the _toBeClosed list and just make it RawDatagramSocket _incoming IPv6.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @cbracken and @jmagman for your time spent in this PR.
I applied the suggested changes. Please keep in mind that I'm not pro in dart/flutter, I did try my best for the tests.
Any code suggestion is really appreciated.
Thanks again 🙇

@biagiopietro biagiopietro changed the title [multicast_dns] Optimized Socket Binding: Consolidated to 0.0.0.0 for Simplicity and Efficiency - #79772 [multicast_dns] Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency - #79772 May 24, 2024
@biagiopietro
Copy link
Author

Apologies for the delay reviewing this.

@vashworth kindly tested this out against the flutter CLI and confirmed it didn't break wireless debugging for flutter run or flutter attach.

This change generally LGTM, though I would like a second opinion from @cbracken who might remember more about why the original code was written that way.

1. This needs a pubspec version bump and CHANGELOG update.  See https://github.com/flutter/flutter/blob/master/docs/ecosystem/contributing/README.md#version-and-changelog-updates

2. Can you add a test to https://github.com/flutter/packages/blob/main/packages/multicast_dns/test/client_test.dart?

@jmagman is it the following command enough for accomplishing point 1? sorry if it's sounds like a stupid question but I ran it locally and it changed changelog and pubspec of almost all the packages.

dart run script/tool/bin/flutter_plugin_tools.dart update-release-info \
  --version=bugfix \
  --base-branch=fix-issue-79772 \
  --changelog="Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency."

@jmagman
Copy link
Member

jmagman commented May 28, 2024

is it the following command enough for accomplishing point 1? sorry if it's sounds like a stupid question but I ran it locally and it changed changelog and pubspec of almost all the packages.

dart run script/tool/bin/flutter_plugin_tools.dart update-release-info \
  --version=bugfix \
  --base-branch=fix-issue-79772 \
  --changelog="Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency."

You could edit the pubspec.yaml and CHANGELOG.md directly.

cc @stuartmorgan re: the update-release-info command, is something missing from the docs? https://github.com/flutter/packages/blob/main/script/tool/README.md#update-changelog-and-version

@stuartmorgan
Copy link
Contributor

stuartmorgan commented May 28, 2024

cc @stuartmorgan re: the update-release-info command, is something missing from the docs? https://github.com/flutter/packages/blob/main/script/tool/README.md#update-changelog-and-version

The docs say to use upstream/main as the base branch in general.

The command run above used this PR's branch as the base branch, which means it will (by definition) find no changes. The repo tooling always interprets no changes as "assume everything changed" as a failsafe to prevent CI from silently running no tests if something goes wrong with diff computation. (Even if it didn't, that command still wouldn't have update the package, it would have just done nothing.)

(The --current-package option documented earlier can also be used in a case like this, instead of --base-branch=....)

@biagiopietro
Copy link
Author

Thanks for the explanation @stuartmorgan / @jmagman , 👇 did the trick:

➜  multicast_dns git:(fix-issue-79772) dart run ../../script/tool/bin/flutter_plugin_tools.dart update-release-info --current-package \
  --version=bugfix \
  --changelog="Optimized Socket Binding: Always bind to 0.0.0.0 for simplicity and efficiency."

@biagiopietro
Copy link
Author

finally all checks are green 😅

@cbracken
Copy link
Member

Taking another look! Thanks!

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment, other than that looks good.

It may be worth adding an equivalent test for IPv6.

for (final RawDatagramSocket socket in _sockets) {
socket.send(packet, _mDnsAddress!, selectedMDnsPort);
if (_incomingIPv4 == null) {
throw StateError('Unable to send packet on null socket.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this cause an exception to be thrown if the connection is over IPv6?

For IPv6 in the old code, we'd just have no sockets to send over and not send anything, but now we'll throw an exception. I think you can just drop this check and everything should be fine, since you're using a null-safe dereference on the send call below.

Copy link
Member

@cbracken cbracken May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why we didn't use equivalent logic for the IPv6 approach in the old code -- i.e. we never sent on the IPv6 equivalent of 0.0.0.0, only on the sockets bound to the individual network interfaces. I'll do a little digging to see if I can understand why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants