diff mbox series

inet: getnameinfo fix serv for abstract socket [BZ #27634]

Message ID 20210327052649.1728350-1-daniel@mariadb.org
State Rejected
Headers show
Series inet: getnameinfo fix serv for abstract socket [BZ #27634] | expand

Commit Message

Daniel Black March 27, 2021, 5:26 a.m. UTC
Abstract sockets were not copied because they began with
\0. They can contain any character, so the full size is used
consistent with how they are created (man 7 unix).
---
 inet/Makefile          |  2 +-
 inet/getnameinfo.c     | 11 +++++++-
 inet/tst-getni-local.c | 63 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 inet/tst-getni-local.c

Comments

Florian Weimer March 27, 2021, 6:57 p.m. UTC | #1
* Daniel Black:

> @@ -459,6 +459,15 @@ gni_serv_local (struct scratch_buffer *tmpbuf,
>  	       const struct sockaddr *sa, socklen_t addrlen,
>  	       char *serv, socklen_t servlen, int flags)
>  {
> +  if (((const struct sockaddr_un *) sa)->sun_path[0] == '\0')
> +    {
> +      /* Abstract socket */
> +      socklen_t len = addrlen - offsetof (struct sockaddr_un, sun_path);
> +      if (len > servlen)
> +	return EAI_OVERFLOW;
> +      memcpy (serv, ((const struct sockaddr_un *) sa)->sun_path, len);
> +      return 0;
> +    }
>    return checked_copy
>      (serv, servlen, ((const struct sockaddr_un *) sa)->sun_path);
>  }

Is this really useful as an interface?  The caller would still have to
know the struct sockaddr_un layout to figure out the length.  So it
could just struct sockaddr_un directly.

In general, getnameinfo itself is not very portable when applied to
AF_UNIX addresses.  Not all systems that have AF_UNIX also implement
it for getnameinfo.  How the path is mapped between the host and
service names also differs.  Therefore, I think applications should
look for AF_UNIX addresses directly.  There is also no string
transformation needed, unlike for the other address families.
Daniel Black March 27, 2021, 10:32 p.m. UTC | #2
Hey Florian,

On Sun, Mar 28, 2021 at 5:57 AM Florian Weimer <fw@deneb.enyo.de> wrote:

> * Daniel Black:
>
> > @@ -459,6 +459,15 @@ gni_serv_local (struct scratch_buffer *tmpbuf,
> >              const struct sockaddr *sa, socklen_t addrlen,
> >              char *serv, socklen_t servlen, int flags)
> >  {
> > +  if (((const struct sockaddr_un *) sa)->sun_path[0] == '\0')
> > +    {
> > +      /* Abstract socket */
> > +      socklen_t len = addrlen - offsetof (struct sockaddr_un, sun_path);
> > +      if (len > servlen)
> > +     return EAI_OVERFLOW;
> > +      memcpy (serv, ((const struct sockaddr_un *) sa)->sun_path, len);
> > +      return 0;
> > +    }
> >    return checked_copy
> >      (serv, servlen, ((const struct sockaddr_un *) sa)->sun_path);
> >  }
>
> Is this really useful as an interface?  The caller would still have to
> know the struct sockaddr_un layout to figure out the length.  So it
> could just struct sockaddr_un directly.
>

Partially, though with the prevalence of systemd socket activation
providing a
neat ListenStream=@abstract name, coders can make broad assumptions
that the only null character is the first.

A simplified version of my use case is attached, where I get a bunch of FDs
that I didn't create and want to log their nature.

Currently I get this:

$  systemd-socket-activate -l @abstract -l /tmp/ss.sock -l 3333
 ./resolve-by-fd
Listening on @abstract as 3.
Listening on /tmp/ss.sock as 4.
Listening on [::]:3333 as 5.
Communication attempt on fd 5.
Execing ./resolve-by-fd (./resolve-by-fd)
Using systemd activated socket :: port 3333
Using systemd activated socket localhost port /tmp/ss.sock
Using systemd activated socket localhost port @tmp/ss.sock

Notably not:
Using systemd activated socket localhost port @abstract

I can quite easily do without the width modifier in my code.

While in the meantime I've got a work around that looks like:

  sbuf[0] == '\0' ? addr.un.sun_path : sbuf

(which isn't terrible).


> In general, getnameinfo itself is not very portable when applied to
> AF_UNIX addresses.  Not all systems that have AF_UNIX also implement
> it for getnameinfo.


Point taken, and I'm looking at pretty much systemd as a Linux only
implementation.

  How the path is mapped between the host and
> service names also differs.


With the portability differences, in the case that a service name happens
to start with \0
(is there one?) there are two possible cases:

* sbuf gets populated with exactly what was in sun_path, I thought this was
pretty portable.
* if NI_MAXSERV wasn't used, you may end up with EAI_OVERFLOW

Therefore, I think applications should
> look for AF_UNIX addresses directly.  There is also no string
> transformation needed, unlike for the other address families.
>

I was just looking to make the most of a single interface.

Thanks for the review and detailed thoughts.
Daniel Black March 28, 2021, 9:55 p.m. UTC | #3
Is this really useful as an interface?  The caller would still have to
> know the struct sockaddr_un layout to figure out the length.  So it
> could just struct sockaddr_un directly.
>

I missed NI_MAXSERV is only 32 which is ok(maybe) for abstract sockets but
real path lengths, not so much.
Florian Weimer March 29, 2021, 8:22 a.m. UTC | #4
* Daniel Black:

>> How the path is mapped between the host and
>> service names also differs.
>
> With the portability differences, in the case that a service name
> happens to start with \0 (is there one?) there are two possible
> cases:
>
> * sbuf gets populated with exactly what was in sun_path, I thought this was
> pretty portable.
> * if NI_MAXSERV wasn't used, you may end up with EAI_OVERFLOW

What I meant is that some implementations put the socket name into the
host name, not the service name.  The length restrictions you
discovered might be related to that.
diff mbox series

Patch

diff --git a/inet/Makefile b/inet/Makefile
index cf4cf5cf..3b0d69be 100644
--- a/inet/Makefile
+++ b/inet/Makefile
@@ -56,7 +56,7 @@  aux := check_pf check_native ifreq
 tests := htontest test_ifindex tst-ntoa tst-ether_aton tst-network \
 	 tst-gethnm test-ifaddrs bug-if1 test-inet6_opt tst-ether_line \
 	 tst-getni1 tst-getni2 tst-inet6_rth tst-checks tst-checks-posix \
-	 tst-sockaddr test-hnto-types tst-if_index-long
+	 tst-sockaddr test-hnto-types tst-if_index-long tst-getni-local
 
 # tst-deadline must be linked statically so that we can access
 # internal functions.
diff --git a/inet/getnameinfo.c b/inet/getnameinfo.c
index 8380d857..58d138d3 100644
--- a/inet/getnameinfo.c
+++ b/inet/getnameinfo.c
@@ -396,7 +396,7 @@  gni_host_local (struct scratch_buffer *tmpbuf,
   return checked_copy (host, hostlen, "localhost");
 }
 
-/* Convert the host part of an AF_LOCAK socket address.   */
+/* Convert the host part of an AF_LOCAL socket address.   */
 static int
 gni_host (struct scratch_buffer *tmpbuf,
 	  const struct sockaddr *sa, socklen_t addrlen,
@@ -459,6 +459,15 @@  gni_serv_local (struct scratch_buffer *tmpbuf,
 	       const struct sockaddr *sa, socklen_t addrlen,
 	       char *serv, socklen_t servlen, int flags)
 {
+  if (((const struct sockaddr_un *) sa)->sun_path[0] == '\0')
+    {
+      /* Abstract socket */
+      socklen_t len = addrlen - offsetof (struct sockaddr_un, sun_path);
+      if (len > servlen)
+	return EAI_OVERFLOW;
+      memcpy (serv, ((const struct sockaddr_un *) sa)->sun_path, len);
+      return 0;
+    }
   return checked_copy
     (serv, servlen, ((const struct sockaddr_un *) sa)->sun_path);
 }
diff --git a/inet/tst-getni-local.c b/inet/tst-getni-local.c
new file mode 100644
index 00000000..8c39fb27
--- /dev/null
+++ b/inet/tst-getni-local.c
@@ -0,0 +1,63 @@ 
+/* Test for getnameinfo AF_LOCAL/UNIX sockets
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <netdb.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <string.h>
+#include <stddef.h>
+
+#include <support/check.h>
+
+#define TEST_SOCK1 "my funky socket"
+static int
+do_test (void)
+{
+  struct sockaddr_un s;
+  char hbuf[NI_MAXHOST], sbuf[NI_MAXSERV];
+  const char abstract[] = { 0, 'a', 'b', 's', 't', 'r', 'a', 'c', 't', 0,
+			   '!' };
+  int len = sizeof(abstract) + offsetof (struct sockaddr_un, sun_path);
+
+  s.sun_family = AF_UNIX;
+  strcpy (s.sun_path, TEST_SOCK1);
+
+  TEST_VERIFY (getnameinfo ((struct sockaddr *) &s, sizeof (s), hbuf,
+			    sizeof (hbuf), sbuf, sizeof (sbuf),
+			    NI_NUMERICHOST | NI_NUMERICSERV) == 0);
+
+  TEST_VERIFY (strncmp ("localhost", hbuf, NI_MAXHOST) == 0);
+  TEST_VERIFY (strncmp (TEST_SOCK1, sbuf, NI_MAXSERV) == 0);
+
+  memset ( hbuf, 0, NI_MAXHOST );
+  memset ( sbuf, 0, NI_MAXSERV );
+
+  memcpy ( s.sun_path, abstract, sizeof (abstract) );
+
+  TEST_VERIFY (getnameinfo ((struct sockaddr *) &s, len, hbuf, sizeof (hbuf),
+			    sbuf, sizeof (sbuf),
+			    NI_NUMERICHOST | NI_NUMERICSERV) == 0);
+
+  TEST_VERIFY (strncmp ("localhost", hbuf, NI_MAXHOST) == 0);
+  TEST_VERIFY (memcmp (abstract, sbuf, sizeof (abstract)) == 0);
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"