Specifying a dbus tcp address without a family let dbus-daemon the choice for listen on ipv4 and/or ipv6 but it did not return the real used ip family, which is fixed with this commit. This bug has been reported at https://bugs.freedesktop.org/show_bug.cgi?id=61922#c56 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Created attachment 138072 [details] [review] Add actual used ip family to --print-address output in case of listening on tcp
Comment on attachment 138072 [details] [review] Add actual used ip family to --print-address output in case of listening on tcp Review of attachment 138072 [details] [review]: ----------------------------------------------------------------- If the dbus-daemon successfully listens on both IPv4 and IPv6, this will return tcp:host=localhost,port=12345,family=ipv4 or tcp:host=localhost,port=12345,family=ipv6 whichever one came out of the resolver first, even though connecting via IPv4 or IPv6 would work equally well. I think it would be better to have this (pseudocode): _dbus_listen_tcp_socket (..., const char **retfamily, ...) { dbus_bool_t have_ipv4 = FALSE; dbus_bool_t have_ipv6 = FALSE; for each address obtained by resolving host { try to bind to that address if successful { if (family == AF_INET) have_ipv4 = TRUE; else if (family == AF_INET6) have_ipv6 = TRUE; } } ... if successful { if (have_ipv4 && !have_ipv6) *retfamily = "ipv4"; else if (!have_ipv4 && have_ipv6) *retfamily = "ipv6"; } } _dbus_server_new_for_tcp_socket { const char *family = NULL; ... _dbus_listen_tcp_socket (..., &family, ...); ... if (family != NULL) { if (!_dbus_string_append (&address, ",family=") || !_dbus_string_append (&address, family)) OOM; } That way, --print-address will print ",family=ipv4" if and only if the only address family we successfully listened on was IPv4, and the same for IPv6. ::: dbus/dbus-server-socket.c @@ +481,5 @@ > dbus_set_error (error, DBUS_ERROR_NO_MEMORY, NULL); > goto failed; > } > + if(!_dbus_string_append (&address, ",family=") || > + !_dbus_string_append (&address, _dbus_string_get_const_data(&family_str))) Coding style: space before (, indent {} and their contents 2 spaces. ::: dbus/dbus-sysdeps-unix.c @@ +1649,4 @@ > listen_fd[nlisten_fd].fd = fd; > nlisten_fd++; > > + if (!_dbus_string_get_length(retfamily)) Similar coding style issues. Also, if the body of an "if" is not just a simple statement, please wrap it in {}: if (foo) { if (bar) ... } is more readable than if (foo) if (bar) ... particularly if there is an "else". ::: dbus/dbus-sysdeps-win.c @@ +1833,4 @@ > listen_fd[nlisten_fd] = fd; > nlisten_fd++; > > + if (!_dbus_string_get_length(retfamily)) Same coding style issues as for Unix.
(In reply to Simon McVittie from comment #2) > Comment on attachment 138072 [details] [review] [review] > Add actual used ip family to --print-address output in case of listening on > tcp > > Review of attachment 138072 [details] [review] [review]: > ----------------------------------------------------------------- > > If the dbus-daemon successfully listens on both IPv4 and IPv6, this will > return > > tcp:host=localhost,port=12345,family=ipv4 > > or > > tcp:host=localhost,port=12345,family=ipv6 > > whichever one came out of the resolver first, even though connecting via > IPv4 or IPv6 would work equally well. Should dbus-daemon not better return two addresses in this case ? tcp:host=localhost,port=12345,family=ipv6;tcp:host=localhost,port=12345,family=ipv4
(In reply to Ralf Habacker from comment #3) > > Review of attachment 138072 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > If the dbus-daemon successfully listens on both IPv4 and IPv6, this will > > return > > > > tcp:host=localhost,port=12345,family=ipv4 > > > > or > > > > tcp:host=localhost,port=12345,family=ipv6 > > > > whichever one came out of the resolver first, even though connecting via > > IPv4 or IPv6 would work equally well. > > Should dbus-daemon not better return two addresses in this case ? > > > tcp:host=localhost,port=12345,family=ipv6;tcp:host=localhost,port=12345, > family=ipv4 The implementation looks to me that it tries to listen to all addresses returned from the resolver, which may be two in case of using AF_UNSPEC. redo_lookup_with_port: getaddrinfo(host, port, &hints, &ai)) tmp = ai; while (tmp) { socket... bind ... listen ... listen_fd = newlisten_fd; listen_fd[nlisten_fd].fd = fd; nlisten_fd++; tmp = tmp->ai_next; }
(In reply to Ralf Habacker from comment #4) > > Should dbus-daemon not better return two addresses in this case ? I don't think so: we already go to some trouble to make sure the various addresses have the same port, so that we can describe them all in a single address by omitting the family. I'm reluctant to put effort into making this any more complicated than it already is, because 99.9% of the time, the address we're listening on should be localhost with AF_UNSPEC, which resolves to 127.0.0.1 and/or ::1. I've been very tempted to make dbus-daemon refuse to listen on non-local addresses (either for the two well-known buses, or always), because doing so is only valid if you completely trust the local LAN (we don't have a security layer like TLS). DBusServer should continue to support non-local TCP, because people use it to build weird distributed systems; but I think dbus-daemon almost certainly shouldn't, because exposing the well-known system bus and the well-known session bus to an untrusted network isn't safe, and on the modern Internet essentially all networks should be treated as untrusted. (Similarly, I think the only reason the well-known buses should support TCP is because Windows doesn't have AF_UNIX, so TCP is our least-bad option for a sockets-based transport that works like you'd expect with select(). On Unix, AF_UNIX is always better.) > The implementation looks to me that it tries to listen to all addresses > returned from the resolver, which may be two in case of using AF_UNSPEC. In theory there can be more than that, if you listen on a DNS name that resolves to multiple addresses, for instance on a "multi-homed" server (whether those multiple addresses are in the same address family or not). Suppose hal9000.example.com resolves to A records 192.168.0.1 and 10.1.2.3, and AAAA records 2001:DB8::9000 and 2001:DB8:4321:1234::1. A machine in the example.com domain might ask to listen on tcp:host=hal9000 and end up listening on 192.168.0.1:12345, 10.1.2.3:12345, [2001:DB8::9000]:12345 and [2001:DB8:4321:1234::1]:12345, which could be validly summarized as tcp:host=hal9000,port=12345.
Created attachment 138159 [details] [review] Add actual used ip family to --print-address output in case of listening on tcp Specifying a dbus tcp address without a family let dbus-daemon the choice for listen on ipv4 or ipv6, but did not return the real used ip family, which is fixed with this commit. - refactored to only return a family in ipv4 or ipv6 case
Comment on attachment 138159 [details] [review] Add actual used ip family to --print-address output in case of listening on tcp Review of attachment 138159 [details] [review]: ----------------------------------------------------------------- Thanks, this looks better. ::: dbus/dbus-server-socket.c @@ +429,4 @@ > DBusString host_str; /* Initialized as const later, not freed */ > DBusString port_str = _DBUS_STRING_INIT_INVALID; > DBusNonceFile *noncefile = NULL; > + const char *family_str = NULL; Coding style nitpicking: Having family and family_str doesn't seem great when they are of the same type. When two variables differ only by a suffix that describes the type, I'd expect them to have the same value and differ only in type. Perhaps this one could be renamed to family_used, which I think would be clearer? ::: dbus/dbus-sysdeps-win.c @@ +1652,4 @@ > * @param port the port to listen on, if zero a free port will be used > * @param family the address family to listen on, NULL for all > * @param retport string to return the actual port listened on > + * @param retfamily string to return the actual family listened on The changes in this file look good, but dbus-sysdeps-unix.c will need the same changes. If you enable Travis-CI for your github clone and push this branch to your clone, I expect you'll see it fail to build in all the non-mingw configurations.
Created attachment 138196 [details] [review] Add actual used ip family to --print-address output in case of listening on tcp Specifying a dbus tcp address without a family let dbus-daemon the choice for listen on ipv4 or ipv6, but did not return the real used ip family, which is fixed with this commit. - renamed family_str to family_used - keep unix implementation of dbus_listen_tcp_socket in sync with windows
Comment on attachment 138196 [details] [review] Add actual used ip family to --print-address output in case of listening on tcp Review of attachment 138196 [details] [review]: ----------------------------------------------------------------- Looks good
applied to master with doc added for param retfamily to unix variant of dbus_listen_tcp_socket
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.