Created attachment 158897 [details] [review]
GUnixSocketAddress: Fix size passed to connect() for abstract sockets

Comment on attachment 158897 [details] [review]
GUnixSocketAddress: Fix size passed to connect() for abstract sockets

First off, the docs for g_socket_address_get_native_size() say "You can use this to allocate memory to pass to g_socket_address_to_native()", and g_unix_socket_address_to_native() currently does:

  sock = (struct sockaddr_un *) dest;
  sock->sun_family = AF_UNIX;
  memset (sock->sun_path, 0, sizeof (sock->sun_path));

so if g_unix_socket_address_get_native_size() is ever going to return less than sizeof(struct sockaddr_un), then this needs to be updated.

Second, this:

>+  if (addr->priv->abstract)
>+    return G_STRUCT_OFFSET(struct sockaddr_un, sun_path) + addr->priv->path_len + 1;
>+  else
>+    return sizeof (struct sockaddr_un);

would seem to contradict unix(7), which says:

       *  abstract: an abstract socket address is distinguished  by  the  fact
          that  sun_path[0] is a null byte (’\0’).  All of the remaining bytes
          in sun_path define the "name" of the socket.

and

          When the address of an
          abstract socket is returned by getsockname(2),  getpeername(2),  and
          accept(2),  its  length  is sizeof(struct sockaddr_un)

and for that matter, the g_unix_socket_address_new_abstract() docs say:

     All bytes after @path_len up to the max size
     of an abstract unix domain name is filled with zero bytes.

(In reply to comment #2)
> (From update of attachment 158897 [details] [review])
> First off, the docs for g_socket_address_get_native_size() say "You can use
> this to allocate memory to pass to g_socket_address_to_native()", and
> g_unix_socket_address_to_native() currently does:
> 
>   sock = (struct sockaddr_un *) dest;
>   sock->sun_family = AF_UNIX;
>   memset (sock->sun_path, 0, sizeof (sock->sun_path));
> 
> so if g_unix_socket_address_get_native_size() is ever going to return less than
> sizeof(struct sockaddr_un), then this needs to be updated.

Hmm, yes.  I guess we can't change _get_native_size.  I think we need to special case the abstract socket one here.  if (G_IS_UNIX_SOCKET_ADDRESS) { length = g_unix_socket_address_get_path_length () } else { length = size; }

Though now that I look closer, DBus doesn't special case how abstract is handled for the length; it always passes the path length.  See:

http://cgit.freedesktop.org/dbus/dbus/tree/dbus/dbus-sysdeps-unix.c#n819

> would seem to contradict unix(7), which says:
> 
>        *  abstract: an abstract socket address is distinguished  by  the  fact
>           that  sun_path[0] is a null byte (’\0’).  All of the remaining bytes
>           in sun_path define the "name" of the socket.

Though my guess is that really you don't want to name every abstract socket with trailing NULs up till UNIX_PATH_MAX, so what's passed to connect() is how many bytes of the address you want to use.
 
> and
> 
>           When the address of an
>           abstract socket is returned by getsockname(2),  getpeername(2),  and
>           accept(2),  its  length  is sizeof(struct sockaddr_un)

Note that doesn't mention connect() though.

(In reply to comment #3)
> Hmm, yes.  I guess we can't change _get_native_size.

well, you could, you'd just have to change _to_native to match it.

> Though now that I look closer, DBus doesn't special case how abstract is
> handled for the length; it always passes the path length.

Yeah, the man page supports that behavior for non-abstract sockets.

> Though my guess is that really you don't want to name every abstract socket
> with trailing NULs up till UNIX_PATH_MAX

Well, it doesn't matter whether or not you *want* to if it's *required*. :-)

> Note that doesn't mention connect() though.

But what happens when you connect() with a short name and then call getpeername()? (From a quick reading of the kernel source code, it looks like the answer is "the man page is wrong, you get back the same length you passed to connect".)


Anyway, if abstract-socket-with-short-name and abstract-socket-with-0-padded-name are not equivalent, and they are both in use, then your patch would break programs that were expecting the current behavior. (Those programs could fix themselves by manually 0-padding their abstract names, but the API docs say they don't have to.) Then again, it's possible that no one is even using this code yet...

(In reply to comment #4)
> Anyway, if abstract-socket-with-short-name and
> abstract-socket-with-0-padded-name are not equivalent,

They can only be equivalent if sizeof(struct sockaddr_un) is always the same. And I don't think it is; I even think Stevens says something that you must never use sizeof(struct sockaddr_un) in portable code.

On my 64-bit Fedora system, sizeof(struct sockaddr_un) turns out to be 110 bytes. Clearly Linux supports names longer than 110 bytes...

> and they are both in
> use, then your patch would break programs that were expecting the current
> behavior. (Those programs could fix themselves by manually 0-padding their
> abstract names, but the API docs say they don't have to.) Then again, it's
> possible that no one is even using this code yet...

But such applications would be broken anyway because they would rely on sizeof(struct sockaddr_un) to be constant. No-one says that won't change from one Linux/libc release to another.

(Of course sizeof(struct sockaddr_un) probably won't change because it just doesn't matter what the size is. This is because you are supposed to use "struct sockaddr_un" as a variable size struct. E.g. never use sizeof on that type.)

(In reply to comment #5)
> On my 64-bit Fedora system, sizeof(struct sockaddr_un) turns out to be 110
> bytes. Clearly Linux supports names longer than 110 bytes...

Or maybe that is really the limit? I don't know. Either way, avoiding exposing the maximum length in GIO might be nice.

(In reply to comment #6)
> (In reply to comment #5)
> > On my 64-bit Fedora system, sizeof(struct sockaddr_un) turns out to be 110
> > bytes. Clearly Linux supports names longer than 110 bytes...
> 
> Or maybe that is really the limit? I don't know. Either way, avoiding exposing
> the maximum length in GIO might be nice.

Apparently:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=net/unix/af_unix.c;h=3d9122e78f41736c7e654cfe7f2482284cdd1dca;hb=HEAD#l200


static int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned *hashp)
{
  if (len <= sizeof(short) || len > sizeof(*sunaddr))
    return -EINVAL;

with sizeof(*sunaddr) being 110 cf.

  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=include/linux/un.h;h=45561c564b8e004bde76a069ce9823f5085226dd;hb=HEAD

so, yeah, we're limited to a 108 bytes for the path... At least on current Linux...

(In reply to comment #4)
> Anyway, if abstract-socket-with-short-name and
> abstract-socket-with-0-padded-name are not equivalent, and they are both in
> use, then your patch would break programs that were expecting the current
> behavior. (Those programs could fix themselves by manually 0-padding their
> abstract names, but the API docs say they don't have to.) Then again, it's
> possible that no one is even using this code yet...

As mentioned on IRC, __unix_find_socket_byname() explicitly compares the lengths for name equality:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=net/unix/af_unix.c;h=3d9122e78f41736c7e654cfe7f2482284cdd1dca;hb=HEAD#l248

FWIW, I'm using this patch

 static gssize
 g_unix_socket_address_get_native_size (GSocketAddress *address)
 {
-  return sizeof (struct sockaddr_un);
+  GUnixSocketAddress *addr = G_UNIX_SOCKET_ADDRESS (address);
+  if (addr->priv->abstract)
+    return G_STRUCT_OFFSET(struct sockaddr_un, sun_path) + addr->priv->path_len + 1;
+  else
+    return G_STRUCT_OFFSET(struct sockaddr_un, sun_path) + addr->priv->path_len;
 }

in GDBus without any problems for connecting to both the system and session buses. The gio test suite also works fine with this patch.

(In reply to comment #9)
> in GDBus without any problems for connecting to both the system and session
> buses. The gio test suite also works fine with this patch.

right, the problem is though, if you have a server that does

  sun.sun_family = AF_UNIX;
  memset (&sun.sun_path, 0, sizeof (sun.sun_path));
  strcpy (sun.sun_path + 1, "my abstract socket name");
  bind (s, &sun, sizeof (sun));
  listen (s);

then a gio-based client that did:

  g_unix_socket_address_new_abstract ("my abstract socket name", -1);

would be able to connect to it currently, but after your patch, it would fail.

Basically, there are two different ways of naming abstract sockets, and our API can currently only support one of them. We can pick which one, but either way, we lose. We need to add another property to GUnixSocketAddress, to say whether to use fixed-length or variable-length abstract socket names.

(In reply to comment #10)
> (In reply to comment #9)
> > in GDBus without any problems for connecting to both the system and session
> > buses. The gio test suite also works fine with this patch.
> 
> right, the problem is though, if you have a server that does
> 
>   sun.sun_family = AF_UNIX;
>   memset (&sun.sun_path, 0, sizeof (sun.sun_path));
>   strcpy (sun.sun_path + 1, "my abstract socket name");
>   bind (s, &sun, sizeof (sun));
>   listen (s);

Do you know of anyone doing this?

> then a gio-based client that did:
> 
>   g_unix_socket_address_new_abstract ("my abstract socket name", -1);
> 
> would be able to connect to it currently, but after your patch, it would fail.
> 
> Basically, there are two different ways of naming abstract sockets, and our API
> can currently only support one of them. We can pick which one, but either way,
> we lose. We need to add another property to GUnixSocketAddress, to say whether
> to use fixed-length or variable-length abstract socket names.

Really? I mean, if a server, like the one you are suggesting decides to use the address "my abstract socket name\0..\0" it is not portable in the first place since the name depends on UNIX_MAX_PATH and POSIX does not specify this value. Of course the app, using abstract namespace, is Linux specific in the first case but that's beside the point.

The thing is that if UNIX_MAX_PATH was to increase from 108 to, say, 208 then things would definitely break unless you kept your client and server builds in lock-step. And no-one really guarantees you this won't happen. So picking addresses with lots of trailing nul-bytes seems like a bug in the _first_ place.

Note that clients can still use gio in such cases (where the address has a lot of trailing nul-bytes) - such clients just need to pass a positive path_len to g_unix_socket_address_new_abstract().

So I don't think we need to throw another property at this problem - we're simply just fixing a bug and this bug-fix might break clients connecting to servers that have a bunch of nul bytes at the end of their name.

Of course if it turns out there are lots of servers with names with trailing nul bytes, then, meh, it makes sense to support it. If not, I don't see any reason to encourage people to write servers with addresses that has a ton of trailing nul bytes. In fact, making undesirable things hard (yet possible - but the client will need to get UNIX_MAX_PATH from somewhere itself) is a good goal.

(In reply to comment #11)
> >   sun.sun_family = AF_UNIX;
> >   memset (&sun.sun_path, 0, sizeof (sun.sun_path));
> >   strcpy (sun.sun_path + 1, "my abstract socket name");
> >   bind (s, &sun, sizeof (sun));
> >   listen (s);
> 
> Do you know of anyone doing this?

Looking at /proc/net/unix on my system the only process doing this is bluez:

 # hexdump -C /proc/net/unix  |grep -4 bluez
 000003d0  74 0a 66 66 66 66 38 38  30 30 32 65 65 34 30 30  |t.ffff88002ee400|
 000003e0  30 30 3a 20 30 30 30 30  30 30 30 32 20 30 30 30  |00: 00000002 000|
 000003f0  30 30 30 30 30 20 30 30  30 31 30 30 30 30 20 30  |00000 00010000 0|
 00000400  30 30 31 20 30 31 20 33  36 35 33 38 20 40 2f 6f  |001 01 36538 @/o|
 00000410  72 67 2f 62 6c 75 65 7a  2f 61 75 64 69 6f 00 00  |rg/bluez/audio..|
 00000420  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
 00000470  00 00 00 00 00 00 00 00  00 0a 66 66 66 66 38 38  |..........ffff88|
 00000480  30 30 35 65 30 64 38 36  38 30 3a 20 30 30 30 30  |005e0d8680: 0000|

Suggested doc patch

  *
  * If @path_len is -1 then @path is assumed to be a zero terminated
  * string (although in general abstract names need not be zero terminated
- * and can have embedded nuls). All bytes after @path_len up to the max size
- * of an abstract unix domain name is filled with zero bytes.
+ * and can have embedded nuls).
+ *
+ * If the name you are trying to connect to is well-known, without
+ * embedded NUL bytes and known to be padded with NUL-bytes at the
+ * end, you cannot pass -1 as the NUL bytes won't be included. Instead
+ * you would do something like this
+ * <programlisting>
+ * gchar temp_path[UNIX_PATH_MAX + 1];
+ * memset (temp_path, '\0', UNIX_PATH_MAX);
+ * g_strlcpy (temp_path, path, UNIX_PATH_MAX);
+ * address = g_unix_socket_address_new_abstract (temp_path, UNIX_PATH_MAX);
+ * </programlisting>
  *
  * Returns: a new #GUnixSocketAddress
  *

(In reply to comment #11)
> Do you know of anyone doing this?

> Really? I mean, if a server, like the one you are suggesting decides to use the
> address "my abstract socket name\0..\0" it is not portable in the first place
> since the name depends on UNIX_MAX_PATH

UNIX_MAX_PATH is specific to gio. It's defined at the top of gunixsocketaddress.c:

#define UNIX_PATH_MAX sizeof (((struct sockaddr_un *) 0)->sun_path)

As for "is anyone doing this", I would assume so, since this is what the man page tells you to do:

       *  abstract: an abstract socket address is distinguished  by  the  fact
          that  sun_path[0] is a null byte (’\0’).  All of the remaining bytes
          in sun_path define the "name" of the socket.   (Null  bytes  in  the
          name have no special significance.)  The name has no connection with
          file system pathnames.  The socket’s address in  this  namespace  is
          given  by the rest of the bytes in sun_path.  When the address of an
          abstract socket is returned by getsockname(2),  getpeername(2),  and
          accept(2),  its  length  is sizeof(struct sockaddr_un), and sun_path
          contains the abstract name.  The abstract socket namespace is a non-
          portable Linux extension.

It's true that, as noted in comment 4, this is not precisely true, but the fact is: anyone who implemented abstract sockets by reading the man page first would be doing it this way.

Created attachment 159122 [details] [review]
Another patch

(In reply to comment #14)
> As for "is anyone doing this", I would assume so, since this is what the man
> page tells you to do:
> 
>        *  abstract: an abstract socket address is distinguished  by  the  fact
>           that  sun_path[0] is a null byte (’\0’).  All of the remaining bytes
>           in sun_path define the "name" of the socket.   (Null  bytes  in  the
>           name have no special significance.)  The name has no connection with
>           file system pathnames.  The socket’s address in  this  namespace  is
>           given  by the rest of the bytes in sun_path.  When the address of an
>           abstract socket is returned by getsockname(2),  getpeername(2),  and
>           accept(2),  its  length  is sizeof(struct sockaddr_un), and sun_path
>           contains the abstract name.  The abstract socket namespace is a non-
>           portable Linux extension.
> 
> It's true that, as noted in comment 4, this is not precisely true, but the fact
> is: anyone who implemented abstract sockets by reading the man page first would
> be doing it this way.

Well, for better or worse, the unix(7) man page (and most Linux man pages for that matter) aren't written by the people who write the code. Not that it matters much though. Anyway, comment 12 indicates that it is not the standard way; in fact only a single process on my system does this. So I think the default behavior, and what we should encourage developers to use, should be to use names without a lot of trailing NUL bytes.

How about this patch?

Created attachment 159200 [details] [review]
GUnixSocketAddress: handle abstract sockets with "short" names

How's this? It leaves the existing method untouched, for backward compat,
and adds a new g_unix_socket_address_new_with_type(). Also, it fixes
g_socket_address_new_from_native() and to_native() to handle both kinds.

(In reply to comment #16)
> Created an attachment (id=159200) [details] [review]
> GUnixSocketAddress: handle abstract sockets with "short" names
> 
> How's this? It leaves the existing method untouched, for backward compat,
> and adds a new g_unix_socket_address_new_with_type(). Also, it fixes
> g_socket_address_new_from_native() and to_native() to handle both kinds.

Sounds OK to me - how about including it in the stable 2.24 branch too? Thanks for looking into this.

Created attachment 159271 [details] [review]
GUnixSocketAddress: handle abstract sockets with non-0-padded names

updated, now actually works :) and with updated test programs

Matthias says no new API in stable, so this will be 2.26 only

(In reply to comment #18)
> Created an attachment (id=159271) [details] [review]
> GUnixSocketAddress: handle abstract sockets with non-0-padded names
> 
> updated, now actually works :) and with updated test programs

Patch works fine in GDBus. Thanks for fixing this. My next GDBus commit will rely on this new API.

> Matthias says no new API in stable, so this will be 2.26 only

OK.

Attachment 159271 [details] pushed as 19d8cc3 - GUnixSocketAddress: handle abstract sockets with non-0-padded names