Bug 105489 - Add actual used ip family to --print-address output in case of listening on tcp
Summary: Add actual used ip family to --print-address output in case of listening on tcp
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-03-13 15:23 UTC by Ralf Habacker
Modified: 2018-03-19 21:26 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Add actual used ip family to --print-address output in case of listening on tcp (6.13 KB, patch)
2018-03-13 15:23 UTC, Ralf Habacker
Details | Splinter Review
Add actual used ip family to --print-address output in case of listening on tcp (4.60 KB, patch)
2018-03-16 20:46 UTC, Ralf Habacker
Details | Splinter Review
Add actual used ip family to --print-address output in case of listening on tcp (6.17 KB, patch)
2018-03-19 13:59 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2018-03-13 15:23:52 UTC
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>
Comment 1 Ralf Habacker 2018-03-13 15:23:55 UTC
Created attachment 138072 [details] [review]
Add actual used ip family to --print-address output in case of listening on tcp
Comment 2 Simon McVittie 2018-03-13 15:50:51 UTC
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.
Comment 3 Ralf Habacker 2018-03-13 15:58:36 UTC
(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
Comment 4 Ralf Habacker 2018-03-14 07:15:15 UTC
(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;
    }
Comment 5 Simon McVittie 2018-03-14 12:23:52 UTC
(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.
Comment 6 Ralf Habacker 2018-03-16 20:46:20 UTC
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 7 Simon McVittie 2018-03-19 12:43:14 UTC
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.
Comment 8 Ralf Habacker 2018-03-19 13:59:12 UTC
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 9 Simon McVittie 2018-03-19 14:05:40 UTC
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
Comment 10 Ralf Habacker 2018-03-19 21:26:32 UTC
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.