Bug 46013

This came up in bug 35230.

_dbus_append_address_from_socket() has the same bug.

(In reply to comment #2)
> _dbus_append_address_from_socket() has the same bug.

Simon, please correct me if I misunderstood.

As I understand, _dbus_append_address_from_socket() and what Lennart mean isn't the same case.

_dbus_append_address_from_socket() does needed to be fixed if there are unescaped characters in the socket address, otherwise, it will fail with "In D-Bus address, character '%c' should have been escaped\n" error.

So I'll have a patch to fix _dbus_append_address_from_socket().


However, as I understand, if _dbus_transport_new_for_domain_socket() escapes the unescaped value (returns by dbus_address_entry_get_value(), which is already unescaped), then the client transport->address will be the escaped path while the server is really listen on the unescaped path. This is inconsistent?

For example, assume applied patches from #bug61303

dbus-daemon listen on xdg-runtime: address while XDG_RUNTIME_DIR=/tmp/example/© (unicode, a9). Then the connectable address maybe something like /tmp/example/©/dbus/user_bus_address,guid=.....

In the client side, we have the same XDG_RUNTIME_DIR environment, or export DBUS_SESSION_BUS_ADDRESS="xdg-runtime:", so we'll connect to dbus-daemon. However, if _dbus_transport_new_for_domain_socket() escapes the address /tmp/example/©/dbus/user_bus_address, then the transport->address will be /tmp/example/%c2%a9/dbus/user_bus_address, this isn't the same as the server listen on.

Created attachment 89514 [details] [review]
[PATCH] _dbus_append_address_from_socket(): escape value got from  socket fd

(In reply to comment #3)
> However, as I understand, if _dbus_transport_new_for_domain_socket() escapes
> the unescaped value (returns by dbus_address_entry_get_value(), which is
> already unescaped), then the client transport->address will be the escaped
> path while the server is really listen on the unescaped path. This is
> inconsistent?

The various transports start from a D-Bus address (what I called a "listenable address" on some other bugs), convert it to an appropriate native address, listen on it, and "return" the actual address where they're listening as a "connectable address" (which might not be the same as the listenable address that they started with). I think Lennart was concerned about the encoding of a native address back into a connectable address.

What we want is something like this (assume you set TMPDIR to /tmp/© for some bizarre reason, on a natively UTF-8 system):

* listenable D-Bus:   unix:tmpdir=/tmp/%c2%a9
* listenable native:  libdbus will create a socket somewhere in /tmp/©
* connectable native: /tmp/©/dbus-o98y3gwsiu
* connectable:        unix:path=/tmp/%c2%a9/dbus-o98y3gwsiu

and for the trivial case where the address is simultaneously listenable and connectable:

* listenable D-Bus:   unix:path=/home/j%c3%b6rg/c%2b%2b-code/dbus_socket
* listenable native:  /home/jörg/c++-code/dbus_socket
* connectable native: /home/jörg/c++-code/dbus_socket
* connectable:        unix:path=/home/j%c3%b6rg/c%2b%2b-code/dbus_socket

and I think the bug is that the last step will incorrectly produce an invalid address like unix:path=/tmp/©/dbus-o98y3gwsiu or unix:path=/home/jörg/c++-code/dbus_socket.

Comment on attachment 89514 [details] [review]
[PATCH] _dbus_append_address_from_socket(): escape value got from  socket fd

Review of attachment 89514 [details] [review]:
-----------------------------------------------------------------

review-

::: dbus/dbus-sysdeps-unix.c
@@ +4059,5 @@
>      case AF_UNIX:
>        if (socket.un.sun_path[0]=='\0')
>          {
> +          if (_dbus_string_append_printf (address, "unix:abstract=%s",
> +                                          dbus_address_escape_value (&(socket.un.sun_path[1]))))

This leaks the escaped string. Instead, append "unix:abstract=", then use _dbus_address_append_escaped() to escape-and-append the path.

@@ +4065,5 @@
>          }
>        else
>          {
> +          if (_dbus_string_append_printf (address, "unix:path=%s",
> +                                          dbus_address_escape_value (socket.un.sun_path)))

Likewise

@@ +4073,5 @@
>      case AF_INET:
>        if (inet_ntop (AF_INET, &socket.ipv4.sin_addr, hostip, sizeof (hostip)))
>          if (_dbus_string_append_printf (address, "tcp:family=ipv4,host=%s,port=%u",
> +                                        dbus_address_escape_value (hostip),
> +                                        ntohs (socket.ipv4.sin_port)))

Likewise, and also unnecessary: stringified IPv4 addresses match [0-9.]+ which is fine in a D-Bus address.

I don't mind whether you do the escaping correctly (same technique as above), or not at all.

@@ +4081,5 @@
>      case AF_INET6:
>        if (inet_ntop (AF_INET6, &socket.ipv6.sin6_addr, hostip, sizeof (hostip)))
>          if (_dbus_string_append_printf (address, "tcp:family=ipv6,host=%s,port=%u",
> +                                        dbus_address_escape_value (hostip),
> +                                        ntohs (socket.ipv6.sin6_port)))

This, on the other hand, *is* necessary, because ":" isn't in the allowed set (at least according to the specification - the library implementation might be more lenient). Please apply the same correction as for Unix.

You might want to move the port before the hostname, so you can do one append_printf followed by one append_escaped.

Created attachment 89531 [details] [review]
[PATCH v2] _dbus_append_address_from_socket(): escape value got from  socket fd

(In reply to comment #6)
> Comment on attachment 89514 [details] [review] [review]
> [PATCH] _dbus_append_address_from_socket(): escape value got from  socket fd
> 
> Review of attachment 89514 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> review-
> 
> ::: dbus/dbus-sysdeps-unix.c
> @@ +4059,5 @@
> >      case AF_UNIX:
> >        if (socket.un.sun_path[0]=='\0')
> >          {
> > +          if (_dbus_string_append_printf (address, "unix:abstract=%s",
> > +                                          dbus_address_escape_value (&(socket.un.sun_path[1]))))
> 
> This leaks the escaped string. Instead, append "unix:abstract=", then use
> _dbus_address_append_escaped() to escape-and-append the path.

Yes, oops! Have to free it after printf! The updated patch take your suggestion.

> 
> @@ +4065,5 @@
> >          }
> >        else
> >          {
> > +          if (_dbus_string_append_printf (address, "unix:path=%s",
> > +                                          dbus_address_escape_value (socket.un.sun_path)))
> 
> Likewise
> 
> @@ +4073,5 @@
> >      case AF_INET:
> >        if (inet_ntop (AF_INET, &socket.ipv4.sin_addr, hostip, sizeof (hostip)))
> >          if (_dbus_string_append_printf (address, "tcp:family=ipv4,host=%s,port=%u",
> > +                                        dbus_address_escape_value (hostip),
> > +                                        ntohs (socket.ipv4.sin_port)))
> 
> Likewise, and also unnecessary: stringified IPv4 addresses match [0-9.]+
> which is fine in a D-Bus address.

Yes, also the hostname limited to [a-zA-Z0-9] and '-', all are valid in dbus address.

I did that just to ensure it. :-), the updated patch doesn't escape it as you suggested.

> 
> I don't mind whether you do the escaping correctly (same technique as
> above), or not at all.
> 
> @@ +4081,5 @@
> >      case AF_INET6:
> >        if (inet_ntop (AF_INET6, &socket.ipv6.sin6_addr, hostip, sizeof (hostip)))
> >          if (_dbus_string_append_printf (address, "tcp:family=ipv6,host=%s,port=%u",
> > +                                        dbus_address_escape_value (hostip),
> > +                                        ntohs (socket.ipv6.sin6_port)))
> 
> This, on the other hand, *is* necessary, because ":" isn't in the allowed
> set (at least according to the specification - the library implementation

Yes, understand.

> might be more lenient). Please apply the same correction as for Unix.
> 
> You might want to move the port before the hostname, so you can do one
> append_printf followed by one append_escaped.

Thanks for your tip.

(In reply to comment #8)
> Yes, also the hostname limited to [a-zA-Z0-9] and '-', all are valid in dbus
> address.

If inet_ntop() could legitimately return a hostname, I'd have said "you can't assume the hostname is syntactically valid"; but that's not what inet_ntop() does, so it's OK anyway.

Comment on attachment 89531 [details] [review]
[PATCH v2] _dbus_append_address_from_socket(): escape value got from  socket fd

Review of attachment 89531 [details] [review]:
-----------------------------------------------------------------

Looks good.

::: dbus/dbus-sysdeps-unix.c
@@ +4060,4 @@
>      case AF_UNIX:
>        if (socket.un.sun_path[0]=='\0')
>          {
> +          _dbus_string_init_const (&path_str, &(socket.un.sun_path[1]));

FYI, the second argument can equivalently be spelled (socket.un.sun_path + 1), which I usually find less ugly. No big deal either way, though.

@@ +4075,5 @@
>        break;
>      case AF_INET:
>        if (inet_ntop (AF_INET, &socket.ipv4.sin_addr, hostip, sizeof (hostip)))
>          if (_dbus_string_append_printf (address, "tcp:family=ipv4,host=%s,port=%u",
> +                                        hostip, ntohs (socket.ipv4.sin_port)))

Unnecessary re-indentation on a line you didn't otherwise touch. I'll merge it, but please read your own diffs and check that they don't do anything unnecessary (like mixing whitespace adjustment with real semantic changes).

I already applied your patch, in fact. Fixed in git for 1.7.10, thanks.

This is a fairly long-standing bug and people want me to release 1.8 soon, so I'm not backporting to 1.6 for now.