seems like a plausible idea, but probably needs at least some control over the socket; what permissions do you create it with? And it's generally useful to have some non-random part of the socket name, so you can figure out whose socket it is when you see it.

also, for the same reason as mkstemp vs mktemp, you would want this to be a GSocket or GSocketConnection constructor, not a GSocketAddress constructor.

What we actually have in telepathy-glib at the moment is this, which avoids "the mktemp problem" by adding the GSocketAddress to a GSocketService straight away; the GSocketAddress is only returned for convenience.

As you say, it'd probably be better if we returned a new GSocket, and had the caller use g_socket_listener_add_socket() if it wanted to. The caller could always get the address (to give to others) via g_socket_get_local_address, but it'd probably be reasonable to have it as an "out" argument for convenience.

GSocketAddress *
_tp_create_temp_unix_socket (GSocketService *service,
    GError **error)
{
  guint i;
  GSocketAddress *address;

  /* why doesn't GIO provide a method to create a socket we don't
   * care about? Iterate until we find a valid temporary name.
   *
   * Try a maximum of 10 times to get a socket */
  for (i = 0; i < 10; i++)
    {
      address = g_unix_socket_address_new (tmpnam (NULL));

      g_clear_error (error);

      if (g_socket_listener_add_address (
            G_SOCKET_LISTENER (service),
            address, G_SOCKET_TYPE_STREAM,
            G_SOCKET_PROTOCOL_DEFAULT,
            NULL, NULL, error))
        return address;
      else
        g_object_unref (address);
    }

  return NULL;
}

Created attachment 172922 [details] [review]
telepathy-glib patch, please s/tp_// for GLib :-)

I've implemented what danw suggested, in telepathy-glib for now: does this look suitable to go in GLib with suitable re-namespacing? I attach my proposed patch for telepathy-glib.

See <https://bugs.freedesktop.org/show_bug.cgi?id=31029>.

In telepathy-glib we keep the ABI the same on all platforms, and we don't have an equivalent of gio-unix, so the function always exists but will fail at runtime if we don't have Unix sockets. I suspect that for GLib you'd want the semantics to be "just doesn't exist on non-Unix", which would be fine too.

Comment on attachment 172922 [details] [review]
telepathy-glib patch, please s/tp_// for GLib :-)

>+ * tp_g_socket_listen_unix:

hm... I slightly dislike having a unix-specific method in gsocket.h. What do you think about flipping it around to:

GSocketAddress *g_unix_socket_address_new_temp (const char *hint, int mode, GSocketType type, GSocket **socket, GError **error);

?

If we do keep it in gsocket, it should still have "_new_" in its name, since it's returning a new GSocket. Also, in that case, it seems like returning the path instead of the GSocketAddress would be more useful to the caller.

>+ * @mode: permissions with which to create the socket, as for g_open()
>+ *  (typically 0700)

I changed my mind about this; generally if you want all users to be able to access a socket, you want it to have a well-known name, so you wouldn't be using this function. And the caller can just chmod it themselves afterward if they really do need something other than the default. So I guess we can get rid of this.

>+  /* set a safe umask: it's process-global state, so err on the side of too
>+   * strict, and fchmod() to the desired mode later. */
>+  old_umask = umask (0x077);

Can't do that in glib. It might mess up other threads / get messed up by other threads. Also, POSIX doesn't specify the behavior of fchmod on a socket, and according to google, it will always give EINVAL on FreeBSD, and will silently do nothing on Solaris.

I guess this is why lots of programs create their temporary sockets in subdirectories of /tmp rather than in /tmp directly? bug 118563 has several variations of g_mkdtemp().

>+      sock = g_socket_new (G_SOCKET_FAMILY_UNIX, type,
>+          G_SOCKET_PROTOCOL_DEFAULT, error);

It ought to be safe to create the socket once, outside of the loop; if bind() fails then the socket is unchanged and it's safe to call bind() on it again.

(ok, so i just noticed that "g_unix_socket_address_new_temp" was your original idea and I recommended against it. :-}. The key is that it has to return the already-bound GSocket.)

(In reply to comment #5)
> (From update of attachment 172922 [details] [review])
> >+ * tp_g_socket_listen_unix:
> 
> hm... I slightly dislike having a unix-specific method in gsocket.h. What do
> you think about flipping it around to:
> 
> GSocketAddress *g_unix_socket_address_new_temp (const char *hint, int mode,
> GSocketType type, GSocket **socket, GError **error);
> 
> ?

I think it should return the GSocket non-optionally, which for least-astonishment means the GSocket ought to be the returned thing.

Could we have

    GSocket *g_unix_socket_new_temp (blah blah blah);

in gunixsocketaddress.h, or something, in a spirit of "this would be a subclass called GUnixSocket if we were super-OO, but we're not"?

> I changed my mind about this; generally if you want all users to be able to
> access a socket, you want it to have a well-known name, so you wouldn't be
> using this function. And the caller can just chmod it themselves afterward if
> they really do need something other than the default. So I guess we can get rid
> of this.

Fair enough, I'd be happy with defaulting to "as private as possible" (but see below).

On the other hand, we want to use this in Telepathy for transient out-of-band sockets used for file transfers and Tubes; we do a handshake on D-Bus to get the address, which is why we don't need it to be well-known. In principle, there's no reason you couldn't do a similar handshake on the system bus, although I agree it'd be weird.

> >+  /* set a safe umask: it's process-global state, so err on the side of too
> >+   * strict, and fchmod() to the desired mode later. */
> >+  old_umask = umask (0x077);
> 
> Can't do that in glib. It might mess up other threads / get messed up by other
> threads.

Yeah, umask is probably a non-starter; a pity, since it seems to be the only thing that's somewhat reliable :-(

> Also, POSIX doesn't specify the behavior of fchmod on a socket, and
> according to google, it will always give EINVAL on FreeBSD, and will silently
> do nothing on Solaris.

Welcome to Unix... I don't suppose chmod() on the path works reliably?

> I guess this is why lots of programs create their temporary sockets in
> subdirectories of /tmp rather than in /tmp directly? bug 118563 has several
> variations of g_mkdtemp().

(I've wished for the API from that bug several times myself; what would it take to make it happen? :-)

I hear rumours that on some non-Linux Unixes, directory permissions don't reliably act on sockets (so you may be able to connect to /badger/mushroom/snake even if badger and/or mushroom is mode 0700 and not yours). I might be getting this and the permissions of the socket itself backwards, though.

Telepathy supports using credentials-passing to stop other local users stealing your file transfers, at least in theory. In practice we only implement it on Linux, the same as older GIO; we could probably make it work on FreeBSD if anyone cares, but not on other platforms.

I wonder whether it'd be best to just remove the permission-related bits, and say "it's OS-specific whether others can access your socket, use credentials-passing or something if you care".

(Counter-intuitively, using IP locally is easy to lock down if you already have a reliable control channel like D-Bus - you can bind() the client socket to 127.0.0.1:0, ask the kernel which port it actually gave you, then tell the server "I'm about to connect to you from 127.0.0.1:54321, please allow it".)

> >+      sock = g_socket_new (G_SOCKET_FAMILY_UNIX, type,
> >+          G_SOCKET_PROTOCOL_DEFAULT, error);
> 
> It ought to be safe to create the socket once, outside of the loop; if bind()
> fails then the socket is unchanged and it's safe to call bind() on it again.

Thanks, that's useful.

(In reply to comment #7)
> Could we have
> 
>     GSocket *g_unix_socket_new_temp (blah blah blah);
> 
> in gunixsocketaddress.h, or something, in a spirit of "this would be a subclass
> called GUnixSocket if we were super-OO, but we're not"?

Maybe... I'll see what other people think.

> Welcome to Unix... I don't suppose chmod() on the path works reliably?

It does. Hm... an attacker can't connect() to the socket before we chmod() it, because we haven't listen()ed yet, and it can't connect after we chmod() it because the permissions will be wrong. So maybe there's not actually a race condition with bind-then-chmod?

> (I've wished for the API from that bug several times myself; what would it take
> to make it happen? :-)

not sure. "make an implementation everyone likes" :)

> I hear rumours that on some non-Linux Unixes, directory permissions don't
> reliably act on sockets (so you may be able to connect to
> /badger/mushroom/snake even if badger and/or mushroom is mode 0700 and not
> yours). I might be getting this and the permissions of the socket itself
> backwards, though.

It appears that on 10-year-old versions of BSD and Solaris, the permissions on the socket itself were ignored, but from what I can find, even then directory permissions were obeyed. (Eg, http://www.securityfocus.com/bid/456/solution suggests using a private directory to work around the vulnerability.)

> Telepathy supports using credentials-passing to stop other local users stealing
> your file transfers, at least in theory. In practice we only implement it on
> Linux, the same as older GIO; we could probably make it work on FreeBSD if
> anyone cares, but not on other platforms.

At some point gio should have support for credentials-passing on all relevant platforms.

There's some code in GDBusServer that could use this, see

 http://git.gnome.org/browse/glib/tree/gio/gdbusserver.c#n667

FWIW, I think I would prefer something like a method on GSocketListener that does all this. This should be fine since that class already has special interfaces for TCP/IP (e.g. g_socket_listener_add_inet_port) - so adding special interfaces for UNIX shouldn't be a problem...

It could look like this:

 GSocketAddress *
 g_socket_listener_add_unix_temporary (GSocketListener         *listener,
                                       GUnixSocketAddressType   address_type,
                                       const gchar             *path_prefix,
                                       gint                     path_len,
                                       gint                     mode,
                                       GError                 **error);

It's important to note that this method a) can fail; and b) does blocking IO. We could also add g_socket_listener_add_unix_temporary_async and g_socket_listener_add_unix_temporary_finish() down the road.

(In reply to comment #10)

I like David's suggestion in general (and it gives us as much control as we need for Telepathy), but:

>  GSocketAddress *
>  g_socket_listener_add_unix_temporary (GSocketListener         *listener,
>                                        GUnixSocketAddressType   address_type,

Is GUnixSocketAddressType available when we don't have gio-unix? (If not, I'd actually be inclined to fix this by moving it into gio proper, much like the way G_UNIX_SOCKET_ADDRESS_ABSTRACT has a value even on Unixes that lack abstract sockets.)

G_UNIX_SOCKET_ADDRESS_ANONYMOUS would presumably be an error?

Is anyone going to use G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED here either, in practice?

>                                        gint                     path_len,

Does this actually make sense? I would have thought it's only relevant for G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED, where we can just say the length is the length of sun_path (a platform-specific constant).

>                                        gint                     mode,

Do we really need this, or can we just assume a best-effort attempt to get 0700?

For consistency with g_socket_listener_add_inet_port we should add GObject *source_object.

Non-abstract Unix sockets also need non-trivial cleanup: the path must be unlink()ed, and if we created a subdirectory, we'll need to rmdir() it. We could either have a cleanup function documented as "only call this with the result of g_socket_listener_add_unix_temporary()", or return some opaque object that knows how to destroy itself, and also "has-a" GSocketAddress.

I'm almost starting to think Telepathy should discard Unix-socket support and just use IPv4 - we know where we are with IPv4, it's entirely portable, and by announcing which port we'll connect from, we can get better protection from other local users than we'd have for Unix sockets. This makes me sad, though.

(In reply to comment #11)
> (In reply to comment #10)
> 
> I like David's suggestion in general (and it gives us as much control as we
> need for Telepathy), but:
> 
> >  GSocketAddress *
> >  g_socket_listener_add_unix_temporary (GSocketListener         *listener,
> >                                        GUnixSocketAddressType   address_type,
> 
> Is GUnixSocketAddressType available when we don't have gio-unix? (If not, I'd
> actually be inclined to fix this by moving it into gio proper, much like the
> way G_UNIX_SOCKET_ADDRESS_ABSTRACT has a value even on Unixes that lack
> abstract sockets.)

Yeah.

(Actually my view is that the gio/gio-unix/gio-win32 split is broken since we can't build gtk-doc docs for e.g. GWin32{Input,Output}Stream when on Unix and for e.g. GUnix{Input,Output}Stream when on Win32. Which is a real problem because that's treating Win32 like second class citizens - e.g. GWin32InputStream is not listed at http://library.gnome.org/devel/gio/unstable/ . Instead I think it'd be much nicer to always have e.g GWin32InputStream even when on UNIX but get a runtime/compile error instead (gtk-doc would define a flag to not run into compile errors for just getting the GType etc.). But meh - off-topic...)

> G_UNIX_SOCKET_ADDRESS_ANONYMOUS would presumably be an error?

Yeah.

> Is anyone going to use G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED here either, in
> practice?

It's not impossible that a specification (like D-Bus) calls for using names padded with NUL bytes (actually it's not specific in the D-Bus spec - probably should be).

> 
> >                                        gint                     path_len,
> 
> Does this actually make sense? I would have thought it's only relevant for
> G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED, where we can just say the length is the
> length of sun_path (a platform-specific constant).

You need this because the @path argument that you pass could have embedded NUL bytes. It's similar in nature to what we're doing here.

 http://library.gnome.org/devel/gio/unstable/GUnixSocketAddress.html#g-unix-socket-address-new-with-type

Btw, talking to Dan on IRC yesterday we thought that it might be nicer to have @hint instead of @path (as your original proposal) and require that @hint doesn't contain any slashes. Then we'd use TMPDIR (via e.g. g_get_tmp_dir()) to prepend. If we do this, we can probably require there are no embedded NUL bytes. 

Or maybe it's nicer to require the full path prefix and just state in the docs that the caller could use g_get_tmp_dir().

Maybe it's fine to do the latter since the function will be really difficult to use anyway. I don't know.

> 
> >                                        gint                     mode,
> 
> Do we really need this, or can we just assume a best-effort attempt to get
> 0700?

It could be that the program actually wants to make it world- or group-readable, no?

> For consistency with g_socket_listener_add_inet_port we should add GObject
> *source_object.

Could do.

> Non-abstract Unix sockets also need non-trivial cleanup: the path must be
> unlink()ed, and if we created a subdirectory, we'll need to rmdir() it. We
> could either have a cleanup function documented as "only call this with the
> result of g_socket_listener_add_unix_temporary()", or return some opaque object
> that knows how to destroy itself, and also "has-a" GSocketAddress.
> 
> I'm almost starting to think Telepathy should discard Unix-socket support and
> just use IPv4 - we know where we are with IPv4, it's entirely portable, and by
> announcing which port we'll connect from, we can get better protection from
> other local users than we'd have for Unix sockets. This makes me sad, though.

(In reply to comment #12)
> > Is anyone going to use G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED here either, in
> > practice?
> 
> It's not impossible that a specification (like D-Bus) calls for using names
> padded with NUL bytes (actually it's not specific in the D-Bus spec - probably
> should be).

Ugh. I'll have a look at GDBus and check what you actually need; if it's just "pad to the length of sun_path" that's a platform-ABI-specific constant and we can just assume it, but if it's "pad to such-and-such a length" then sadly we do need path_len (but only for that one socket type).

> Btw, talking to Dan on IRC yesterday we thought that it might be nicer to have
> @hint instead of @path (as your original proposal) and require that @hint
> doesn't contain any slashes. Then we'd use TMPDIR (via e.g. g_get_tmp_dir()) to
> prepend. If we do this, we can probably require there are no embedded NUL
> bytes.

I strongly prefer this; see Comment #0, where Guillaume explains that we really don't care where our socket goes, we just want to make a socket without calling tmpnam(NULL) like we do now :-)

In the current telepathy-glib implementation we accept an arbitrary hint, and do the equivalent of `tr -c A-Za-z0-9 _` so only letters, numbers and underscores will appear in the filesystem. After all, it's only a hint to help you work out whose failure-to-unlink has left behind /tmp/MyAwesomeApp-t3hge3hiu or whatever :-)

> Maybe it's fine to do the latter since the function will be really difficult to
> use anyway. I don't know.

It wouldn't say this function is *really* difficult to use; it's certainly easier than not having it :-P

> > >                                        gint                     mode,
> > 
> > Do we really need this, or can we just assume a best-effort attempt to get
> > 0700?
> 
> It could be that the program actually wants to make it world- or
> group-readable, no?

It could be... but enforcement of permissions seems to be non-portable in any case (see above), and not having the mode covers the 99% case.

(In reply to comment #13)
> (In reply to comment #12)
> > > Is anyone going to use G_UNIX_SOCKET_ADDRESS_ABSTRACT_PADDED here either, in
> > > practice?
> > 
> > It's not impossible that a specification (like D-Bus) calls for using names
> > padded with NUL bytes (actually it's not specific in the D-Bus spec - probably
> > should be).
> 
> Ugh. I'll have a look at GDBus and check what you actually need; 

Actually GDBus does not rely on this. I was just trying to say that a) the D-Bus spec is not clear on this (it's not clear on a lot of things); and b) other "frameworks" and/or "specs" does use abstract names with trailing NUL bytes (bluez is one example).

Especially for b) it's not far-fetched that if you want to implement some extension for Framework XYZ... the way to do that is to listen on a socket in the abstract name space... then you send the name to a "master process". The "master process" will then take this name (a NUL-terminated string with no trailing NUL bytes) and pad it with NUL bytes itself and connect to the socket. For this to work you need to listen on a socket with trailing NUL bytes.

> if it's just
> "pad to the length of sun_path" that's a platform-ABI-specific constant and we
> can just assume it, but if it's "pad to such-and-such a length" then sadly we
> do need path_len (but only for that one socket type).
> 
> > Btw, talking to Dan on IRC yesterday we thought that it might be nicer to have
> > @hint instead of @path (as your original proposal) and require that @hint
> > doesn't contain any slashes. Then we'd use TMPDIR (via e.g. g_get_tmp_dir()) to
> > prepend. If we do this, we can probably require there are no embedded NUL
> > bytes.
> 
> I strongly prefer this; see Comment #0, where Guillaume explains that we really
> don't care where our socket goes, we just want to make a socket without calling
> tmpnam(NULL) like we do now :-)
> 
> In the current telepathy-glib implementation we accept an arbitrary hint, and
> do the equivalent of `tr -c A-Za-z0-9 _` so only letters, numbers and
> underscores will appear in the filesystem. After all, it's only a hint to help
> you work out whose failure-to-unlink has left behind
> /tmp/MyAwesomeApp-t3hge3hiu or whatever :-)

In GDBus I need a bit more since this part of the D-Bus specification

 http://dbus.freedesktop.org/doc/dbus-specification.html#transports-unix-domain-sockets

calls for support address of the form unix:tmpdir=/random. Which works like this

 $ ./gdbus-example-peer --server --address unix:tmpdir=/random
 Server is listening at: unix:abstract=/random/dbus-un3r7wGJ

so I really need some kind of control here - especially since non-Linux does not support abstract sockets so the path need to be writable by the process. Actually the last point is a good reason that we probably shouldn't use @hint and should use a full @path_prefix instead.. I'd feel more comfortable just punting it to the user of the API to call g_get_tmp_dir() himself. I don't know.

 (of course this "requirement" in the D-Bus spec is pretty arbitrary. It's
  also perverse how the provider of the D-Bus address have no say in
  whether an abstract socket is created or not...)

(Btw, we also need the API to take a GCancellable - and we should check this in the inner retry loop.)

(In reply to comment #14)
> Especially for b) it's not far-fetched that if you want to implement some
> extension for Framework XYZ... the way to do that is to listen on a socket in
> the abstract name space... then you send the name to a "master process". The
> "master process" will then take this name (a NUL-terminated string with no
> trailing NUL bytes) and pad it with NUL bytes itself and connect to the socket.

I'd be somewhat inclined to say Framework XYZ is (at best) not the 99% case and (at worst) wrong; if using padded abstract socket names, the listener should send a pre-padded byte blob (as an 'ay' if using D-Bus) and the connecter shouldn't second-guess it. But maybe that's just me...

For filesystem sockets (which are all I've actually implemented so far!), this all goes away. Perhaps that's a hint that we should be implementing one convenience method for filesystem sockets, and another for abstract sockets?

> In GDBus I need a bit more since this part of the D-Bus specification
> 
> http://dbus.freedesktop.org/doc/dbus-specification.html#transports-unix-domain-sockets
> 
> calls for support address of the form unix:tmpdir=/random. Which works like
> this
> 
>  $ ./gdbus-example-peer --server --address unix:tmpdir=/random
>  Server is listening at: unix:abstract=/random/dbus-un3r7wGJ

I don't think your use case is necessarily the same as our use case. Yours needs strictly more control, so the function we want for Telepathy could be implemented as a simplified wrapper around the function you want for GDBus, perhaps?

See Comment #0 for how simple our requirement was at the beginning :-)

>  (of course this "requirement" in the D-Bus spec is pretty arbitrary. It's
>   also perverse how the provider of the D-Bus address have no say in
>   whether an abstract socket is created or not...)

Yeah, we avoided that in Telepathy by declaring "Unix" and "abstract Unix" to be distinct socket types.

> (Btw, we also need the API to take a GCancellable - and we should check this in
> the inner retry loop.)

That's a good point.

For reference, here's our current prototype in telepathy-glib:

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=blob;f=telepathy-glib/gnio-util.c;h=f22722e6f176f0fea8d2760a571173064d204436;hb=99edd4b0d376968e68eab08ab12eb5fb70d22feb#l578

I became unhappy about the need for an "unlink my socket" function that only works on addresses returned by the temporary-listening function, so I've ended up with this updated prototype:

    typedef struct _TpUnixListener TpUnixListener;

    TpUnixListener *tp_unix_listener_new (const gchar *hint,
        GSocketType type, GCancellable *cancellable, GError **error);

    GSocket *tp_unix_listener_get_socket (TpUnixListener *self);

    /* really returns a GUnixSocketAddress, transfer none */
    GSocketAddress *tp_unix_listener_get_address (TpUnixListener *self);

    const gchar *tp_unix_listener_get_path (TpUnixListener *self, gsize *len);

    void tp_unix_listener_free (TpUnixListener *self);

This turns the potential filename leak (which we don't really have tools for) into a memory leak of the structure (which we have plenty of tool support for avoiding).

The socket can either be given to a GSocketListener or used with g_socket_accept; I think it seems right to do that rather than enforcing use of GSocketListener, but if you disagree, the constructor could become a getter-style method on a GSocketListener. (For the prototype in telepathy-glib it's certainly easier to have an independent pseudo-object.)

I don't propose to include support for abstract sockets (either flavour). If we want those, we could have tp_unix_listener_new_abstract[_padded], for instance - I think having separate constructors for path-based, abstract and padded-abstract is probably correct.

I don't propose to include support for non-0700 permissions because I think the 99% case is that you want a user-scoped socket. If we want those, we could have a tp_unix_listener_new_full that takes the mode.

Similarly, I didn't include support for a full path prefix, only a "hint" to be used as the beginning of a temporary directory name), because I don't think that's the 99% case. If we want that for GDBus, we could have it in tp_unix_listener_new_full.

I did add GCancellable support.

Thoughts?

Adjusting component to gio in preparation for GitLab migration

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/355.