Re: [PATCH] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

Pedro Alves
2018-10-09T18:02:20Z

On 10/09/2018 06:32 PM, John Darrington wrote:
> When invoking gdbserver, if the COMM parameter does not include a colon (:) and
> is not the name of an existing character device, then a local (unix) domain
> socket will be created with that name and gdbserver will listen for connections
> on that.

Is that "colon/no-colon" magic something that tools frequently do?
Off hand it doesn't seem like a good idea to me, because if you typo a
character device name or unix tcp host name, you end up creating a
unix socket. I'd think an explicit option would be better. E.g.,
reuse gdb's connection specs, i.e., a prefix, like:

gdbserver unix:/tmp/some/path

which can be extended just like gdb's:

gdbserver tcp:host:port
gdbserver tcp6:host:port

etc.

That would suggest extending parse_connection_spec to support "unix" specs.

>
> gdb/doc/
> * gdb.texinfo (Server): Describe connection over a local domain socket.
>
> gdb/gdbserver/
> * NEWS: Mention new feature.

NEWS is under gdb/, not gdb/gdbserver/.

> * configure.ac (AC_CHECK_HEADERS): Add sys/un.h.
> * remote-utils.c (remote_prepare): Create a local socket if requested.
> * remote-utils.c (remote_open): Don't attempt to open a file if it's a socket.
> * remote-utils.c (handle_accept_event): Display the name of the socket on connection.

Don't repeat "* remote-utils.c" for the second and third functions.

Add "* configure: Regenerate.".

> ---
> gdb/NEWS | 4 ++
> gdb/doc/gdb.texinfo | 26 +++++--
> gdb/gdbserver/configure.ac | 2 +-
> gdb/gdbserver/remote-utils.c | 164 ++++++++++++++++++++++++++++++-------------
> 4 files changed, 141 insertions(+), 55 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 78d20713a8..95c5083d9b 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -15,6 +15,10 @@
> can be passed using the '[ADDRESS]:PORT' notation, or the regular
> 'ADDRESS:PORT' method.
>
> +* GDB and GDBserver now support local domain socket connections. The
> + name of a local domain socket may be provided instead of the
> + [ADDRESS]:PORT notation.
> +
> * DWARF index cache: GDB can now automatically save indices of DWARF
> symbols on disk to speed up further loading of the same binaries.
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index efd6dffb1e..44b319ebc7 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -21075,9 +21075,13 @@ syntax is:
> target> gdbserver @var{comm} @var{program} [ @var{args} @dots{} ]
> @end smallexample
>
> -@var{comm} is either a device name (to use a serial line), or a TCP
> +@var{comm} is either an existing device name (to use a serial line), or a TCP
> hostname and portnumber, or @code{-} or @code{stdio} to use
> stdin/stdout of @code{gdbserver}.
> +If @var{comm} is none of the above, then a local domain socket
> +will be created with that name.
> +@cindex local socket
> +@cindex Unix domain socket
> For example, to debug Emacs with the argument
> @samp{foo.txt} and communicate with @value{GDBN} over the serial port
> @file{/dev/com1}:
> @@ -21107,6 +21111,20 @@ conflicts with another service, @code{gdbserver} prints an error message
> and exits.} You must use the same port number with the host @value{GDBN}
> @code{target remote} command.
>
> +If the target and local machine are one and the same, then you can
> +use local domain socket instead of a TCP connection:
> +
> +@smallexample
> +target> gdbserver /tmp/local-socket emacs foo.txt
> +@end smallexample
> +
> +A local domain socket called @file{/tmp/local-socket} will be created
> +in the filesystem.
> +If there is already a local domain socket with this name it will be removed.
> +@smallexample
> +(gdb) target remote /tmp/local-socket
> +@end smallexample
> +
> The @code{stdio} connection is useful when starting @code{gdbserver}
> with ssh:
>
> @@ -21155,10 +21173,10 @@ In case more than one copy of @var{program} is running, or @var{program}
> has multiple threads, most versions of @code{pidof} support the
> @code{-s} option to only return the first process ID.
>
> -@subsubsection TCP port allocation lifecycle of @code{gdbserver}
> +@subsubsection Socket lifecycle of @code{gdbserver}
>
> This section applies only when @code{gdbserver} is run to listen on a TCP
> -port.
> +port or a unix domain socket.
>
> @code{gdbserver} normally terminates after all of its debugged processes have
> terminated in @kbd{target remote} mode. On the other hand, for @kbd{target
> @@ -21174,7 +21192,7 @@ Such reconnecting is useful for features like @ref{disconnected tracing}. For
> completeness, at most one @value{GDBN} can be connected at a time.
>
> @cindex @option{--once}, @code{gdbserver} option
> -By default, @code{gdbserver} keeps the listening TCP port open, so that
> +By default, @code{gdbserver} keeps the listening socket open, so that
> subsequent connections are possible. However, if you start @code{gdbserver}
> with the @option{--once} option, it will stop listening for any further
> connection attempts after connecting to the first @value{GDBN} session. This
> diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
> index fa3ca53efd..de7e7d4103 100644
> --- a/gdb/gdbserver/configure.ac
> +++ b/gdb/gdbserver/configure.ac
> @@ -98,7 +98,7 @@ ACX_CONFIGURE_DIR(["../../libiberty"], ["build-libiberty-gdbserver"])
> AC_CHECK_HEADERS(termios.h sys/reg.h string.h dnl
> proc_service.h sys/procfs.h linux/elf.h dnl
> fcntl.h signal.h sys/file.h dnl
> - sys/ioctl.h netinet/in.h sys/socket.h netdb.h dnl
> + sys/ioctl.h netinet/in.h sys/socket.h sys/un.h netdb.h dnl
> netinet/tcp.h arpa/inet.h)
> AC_FUNC_FORK
> AC_CHECK_FUNCS(getauxval pread pwrite pread64 setns)
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 9199a9c7ad..00e3340156 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -38,6 +38,9 @@
> #if HAVE_NETINET_IN_H
> #include <netinet/in.h>
> #endif
> +#if HAVE_SYS_UN_H
> +#include <sys/un.h>
> +#endif
> #if HAVE_SYS_SOCKET_H
> #include <sys/socket.h>
> #endif
> @@ -193,20 +196,35 @@ handle_accept_event (int err, gdb_client_data client_data)
> descriptor open for add_file_handler to wait for a new connection. */
> delete_file_handler (listen_desc);
>
> - /* Convert IP address to string. */
> - char orig_host[GDB_NI_MAX_ADDR], orig_port[GDB_NI_MAX_PORT];
> -
> - int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> - orig_host, sizeof (orig_host),
> - orig_port, sizeof (orig_port),
> - NI_NUMERICHOST | NI_NUMERICSERV);
> + if (sockaddr.ss_family == AF_UNIX)
> + {
> + struct sockaddr_un su;
> + socklen_t len;
> + if (0 != getsockname (listen_desc, (struct sockaddr *) &su, &len))

We don't use that "constant on lhs style". Put the 0 != on the rhs.

> + {
> + perror (_("Could not obtain remote address"));
> + }

Remove unnecessary curly braces.

>
> - if (r != 0)
> - fprintf (stderr, _("Could not obtain remote address: %s\n"),
> - gai_strerror (r));
> + fprintf (stderr, _("Remote debugging on local socket bound to %s\n"),
> + su.sun_path);
> + }
> else
> - fprintf (stderr, _("Remote debugging from host %s, port %s\n"),
> - orig_host, orig_port);
> + {
> + /* Convert IP address to string. */
> + char orig_host[GDB_NI_MAX_ADDR], orig_port[GDB_NI_MAX_PORT];
> +
> + int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> + orig_host, sizeof (orig_host),
> + orig_port, sizeof (orig_port),
> + NI_NUMERICHOST | NI_NUMERICSERV);
> +
> + if (r != 0)
> + fprintf (stderr, _("Could not obtain remote address: %s\n"),
> + gai_strerror (r));
> + else
> + fprintf (stderr, _("Remote debugging from host %s, port %s\n"),
> + orig_host, orig_port);
> + }
>
> enable_async_notification (remote_desc);
>
> @@ -250,6 +268,9 @@ remote_prepare (const char *name)
> struct addrinfo hint;
> struct addrinfo *ainfo;
>
> + struct sockaddr *addr;
> + socklen_t addrlen;
> +
> memset (&hint, 0, sizeof (hint));
> /* Assume no prefix will be passed, therefore we should use
> AF_UNSPEC. */
> @@ -260,10 +281,17 @@ remote_prepare (const char *name)
> parsed_connection_spec parsed
> = parse_connection_spec_without_prefix (name, &hint);
>
> + struct stat statbuf;
> + int stat_result = stat (name, &statbuf);
> +
> if (parsed.port_str.empty ())
> {
> - cs.transport_is_reliable = 0;
> - return;
> + if (stat_result == 0
> + && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode)))
> + {
> + cs.transport_is_reliable = 0;
> + return;
> + }
> }
>
> #ifdef USE_WIN32API
> @@ -276,47 +304,82 @@ remote_prepare (const char *name)
> }
> #endif
>
> - int r = getaddrinfo (parsed.host_str.c_str (), parsed.port_str.c_str (),
> - &hint, &ainfo);
> + struct sockaddr_un unix_addr;
>
> - if (r != 0)
> - error (_("%s: cannot resolve name: %s"), name, gai_strerror (r));
> +#ifndef UNIX_PATH_MAX
> +#define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
> +#endif
>
> - scoped_free_addrinfo freeaddrinfo (ainfo);
> + if (parsed.port_str.empty ())
> + {
> + if (strlen (name) > UNIX_PATH_MAX - 1)
> + {
> + error
> + (_("%s is too long. Socket names may be no longer than %s bytes."),
> + name, pulongest (UNIX_PATH_MAX - 1));
> + return;
> + }
> + listen_desc = socket (AF_UNIX, SOCK_STREAM, 0);
> + if (listen_desc < 0)
> + perror_with_name ("Can't open socket");
> +
> + memset (&unix_addr, 0, sizeof (unix_addr));
> + unix_addr.sun_family = AF_UNIX;
> + strncpy (unix_addr.sun_path, parsed.host_str.c_str(), UNIX_PATH_MAX - 1);
> + if (stat_result == 0
> + && (S_IFSOCK & statbuf.st_mode))
> + unlink (name);
> +
> + addr = (struct sockaddr *) &unix_addr;
> + addrlen = sizeof (unix_addr);
> + }
> + else
> + {
> + struct addrinfo *iter;
>
> - struct addrinfo *iter;
> + int r = getaddrinfo (parsed.host_str.c_str (),
> + parsed.port_str.c_str (),
> + &hint, &ainfo);
>
> - for (iter = ainfo; iter != NULL; iter = iter->ai_next)
> - {
> - listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype,
> - iter->ai_protocol);
> + if (r != 0)
> + error (_("%s: cannot resolve name: %s"), name, gai_strerror (r));
>
> - if (listen_desc >= 0)
> - break;
> - }
> + scoped_free_addrinfo freeaddrinfo (ainfo);
>
> - if (iter == NULL)
> - perror_with_name ("Can't open socket");
> + for (iter = ainfo; iter != NULL; iter = iter->ai_next)
> + {
> + listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype,
> + iter->ai_protocol);
>
> - /* Allow rapid reuse of this port. */
> - tmp = 1;
> - setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> - sizeof (tmp));
> + if (listen_desc >= 0)
> + break;
> + }
>
> - switch (iter->ai_family)
> - {
> - case AF_INET:
> - ((struct sockaddr_in *) iter->ai_addr)->sin_addr.s_addr = INADDR_ANY;
> - break;
> - case AF_INET6:
> - ((struct sockaddr_in6 *) iter->ai_addr)->sin6_addr = in6addr_any;
> - break;
> - default:
> - internal_error (__FILE__, __LINE__,
> - _("Invalid 'ai_family' %d\n"), iter->ai_family);
> + if (iter == NULL)
> + perror_with_name ("Can't open socket");
> +
> + /* Allow rapid reuse of this port. */
> + tmp = 1;
> + setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> + sizeof (tmp));
> +
> + switch (iter->ai_family)
> + {
> + case AF_INET:
> + ((struct sockaddr_in *) iter->ai_addr)->sin_addr.s_addr = INADDR_ANY;
> + break;
> + case AF_INET6:
> + ((struct sockaddr_in6 *) iter->ai_addr)->sin6_addr = in6addr_any;
> + break;
> + default:
> + internal_error (__FILE__, __LINE__,
> + _("Invalid 'ai_family' %d\n"), iter->ai_family);
> + }
> + addr = iter->ai_addr;
> + addrlen = iter->ai_addrlen;
> }
>
> - if (bind (listen_desc, iter->ai_addr, iter->ai_addrlen) != 0)
> + if (bind (listen_desc, addr, addrlen) != 0)
> perror_with_name ("Can't bind address");
>
> if (listen (listen_desc, 1) != 0)
> @@ -334,6 +397,9 @@ remote_open (const char *name)
> const char *port_str;
>
> port_str = strchr (name, ':');
> + struct stat statbuf;
> + int stat_result = stat (name, &statbuf);
> +
> #ifdef USE_WIN32API
> if (port_str == NULL)
> error ("Only HOST:PORT is supported on this platform.");
> @@ -353,12 +419,10 @@ remote_open (const char *name)
> add_file_handler (remote_desc, handle_serial_event, NULL);
> }
> #ifndef USE_WIN32API
> - else if (port_str == NULL)
> + else if (port_str == NULL && stat_result == 0
> + && 0 == (S_IFSOCK & statbuf.st_mode))

Constant on rhs.

> {
> - struct stat statbuf;
> -
> - if (stat (name, &statbuf) == 0
> - && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode)))
> + if (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode))
> remote_desc = open (name, O_RDWR);
> else
> {
>

Thanks,
Pedro Alves