From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107088 invoked by alias); 18 Oct 2018 20:18:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 107072 invoked by uid 89); 18 Oct 2018 20:18:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=PORT, socketh, UD:socket.h, socket.h X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 18 Oct 2018 20:18:50 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0A98A3001636; Thu, 18 Oct 2018 20:18:49 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id C34EF611C7; Thu, 18 Oct 2018 20:18:48 +0000 (UTC) From: Sergio Durigan Junior To: John Darrington Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested. References: <20181013175801.2670-1-john@darrington.wattle.id.au> Date: Thu, 18 Oct 2018 20:18:00 -0000 In-Reply-To: <20181013175801.2670-1-john@darrington.wattle.id.au> (John Darrington's message of "Sat, 13 Oct 2018 19:57:58 +0200") Message-ID: <87a7nbxb2v.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2018-10/txt/msg00401.txt.bz2 On Saturday, October 13 2018, John Darrington wrote: > When invoking gdbserver, if the COMM parameter takes the form "unix::/path/name" > then a local (unix) domain socket will be created with that name and gdbserver > will listen for connections on that. Thanks for the patch. > gdb/ > * NEWS: Mention new feature. > > gdb/gdbserver/ > * configure.ac (AC_CHECK_HEADERS): Add sys/un.h. > * configure: Regenerate. > * remote-utils.c (remote_prepare): Create a local socket if requested. > (remote_open): Don't attempt to open a file if it's a socket. > (handle_accept_event): Display the name of the socket on connection. > > gdb/common/ > * netstuff.c (parse_connection_spec)[prefixes]: New member for local domain sockets. > --- > gdb/NEWS | 4 ++ > gdb/common/netstuff.c | 9 +++ > gdb/gdbserver/configure.ac | 2 +- > gdb/gdbserver/remote-utils.c | 148 ++++++++++++++++++++++++++++++------------- > 4 files changed, 117 insertions(+), 46 deletions(-) > > diff --git a/gdb/NEWS b/gdb/NEWS > index 126e61e282..23de349324 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. Is it worth explicitly mentioning "UNIX domain socket"? > + > * 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/common/netstuff.c b/gdb/common/netstuff.c > index c1c401ccb2..8da6aba24e 100644 > --- a/gdb/common/netstuff.c > +++ b/gdb/common/netstuff.c > @@ -57,6 +57,8 @@ parse_connection_spec_without_prefix (std::string spec, struct addrinfo *hint) > || std::count (spec.begin (), > spec.end (), ':') > 1))); > > + bool is_unix = hint->ai_family == AF_UNIX; No need for a newline between the declarations of is_ipv6 and is_unix. Here, and everywhere else, AF_UNIX may be undefined if building GDB on a non-UNIX environment. I'm afraid you may have to guard this code with "HAVE_SYS_UN_H". > + > if (is_ipv6) > { > if (spec[0] == '[') > @@ -109,6 +111,12 @@ parse_connection_spec_without_prefix (std::string spec, struct addrinfo *hint) > if (ret.host_str.empty ()) > ret.host_str = "localhost"; > > + if (is_unix && ret.host_str != "localhost") > + error (_("The host name must be empty or 'localhost' for a unix domain socket.")); This line has more than 80 chars, so you will need to break it. Also, IIRC the official name for the system is "UNIX" (all caps). > + > + if (is_unix && ret.port_str.empty ()) > + error (_("A path name must be specified for a unix domain socket.")); So "port_str" is actually a path name if we're dealing with a UNIX socket? What do you think about changing the name of the field to "resource_str" or some such? > + > return ret; > } > > @@ -138,6 +146,7 @@ parse_connection_spec (const char *spec, struct addrinfo *hint) > { "tcp4:", AF_INET, SOCK_STREAM }, > { "udp6:", AF_INET6, SOCK_DGRAM }, > { "tcp6:", AF_INET6, SOCK_STREAM }, > + { "unix:", AF_LOCAL, SOCK_STREAM }, The comment I made about AF_UNIX above also applies to AF_LOCAL. > }; > > for (const host_prefix prefix : prefixes) > 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) The change for "gdb/gdbserver/configure" hasn't been included in the patch. Perhaps you forgot to regenerate it? > diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c > index 9199a9c7ad..0eb53c2116 100644 > --- a/gdb/gdbserver/remote-utils.c > +++ b/gdb/gdbserver/remote-utils.c > @@ -38,6 +38,9 @@ > #if HAVE_NETINET_IN_H > #include > #endif > +#if HAVE_SYS_UN_H > +#include > +#endif > #if HAVE_SYS_SOCKET_H > #include > #endif > @@ -193,20 +196,33 @@ 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; Missing newline here. According to getsockname(2), 'len' should contain the initial size of the 'struct sockaddr' you're passing in the second argument. Therefore, you should declare 'len' as 'sizeof (su)'. Also, there's another 'len' declare in the scope above. In order to avoid -Wshadow warnings, I'd recommend you to choose another variable name here. Last, but not least, this code also needs to be guarded with #ifdef's. > + if (getsockname (listen_desc, (struct sockaddr *) &su, &len) != 0) > + perror (_("Could not obtain remote address")); > > - 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 +266,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. */ > @@ -258,7 +277,7 @@ remote_prepare (const char *name) > hint.ai_protocol = IPPROTO_TCP; > > parsed_connection_spec parsed > - = parse_connection_spec_without_prefix (name, &hint); > + = parse_connection_spec (name, &hint); There's a problem here: gdbserver doesn't work with UDP connections. Actually, this is the only reason why there's a "_without_prefix" variant of "parse_connection_spec": because, in this particular case, we *don't* want to consider prefixes. Because (so far) gdbserver deals only with TCP sockets, and because IPv4 and IPv6 addresses are easily distinguishable, we don't use any kind of prefix. However, with your patch, we will probably need a prefix indeed (to differentiate between serial tty devices and UNIX sockets, at least). My suggestion is that you make a "parse_connection_spec_maybe_unix" or some such, which takes into account *just* the UNIX prefix (and no other). After you extract the "unix:" prefix, you can pass the rest of the string to "parse_connection_spec_without_prefix" (which would be made "static" since it won't be used externally anymore). > > if (parsed.port_str.empty ()) > { > @@ -276,47 +295,86 @@ 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) ^ Missing whitespace between function and parens. > +#endif Even though I don't quite like using "PATH_MAX"-like because of reproducibility issues, I don't really know what else could be done here to prevent that. > > - scoped_free_addrinfo freeaddrinfo (ainfo); > + if (hint.ai_family == AF_LOCAL) > + { > + const char *sock_name = parsed.port_str.c_str (); > + if (parsed.port_str.length() > UNIX_PATH_MAX - 1) ^ Missing whitespace between function and parens. > + { > + error > + (_("%s is too long. Socket names may be no longer than %s bytes."), s/bytes/characters/ (increases readability, IMHO)? > + sock_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"); > > - struct addrinfo *iter; > + memset (&unix_addr, 0, sizeof (unix_addr)); > + unix_addr.sun_family = AF_UNIX; > + strncpy (unix_addr.sun_path, sock_name, UNIX_PATH_MAX - 1); > > - for (iter = ainfo; iter != NULL; iter = iter->ai_next) > - { > - listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype, > - iter->ai_protocol); > + struct stat statbuf; > + int stat_result = stat (sock_name, &statbuf); Missing newline here. No need for the "stat_result" variable; you can just check its return value in the "if" below. > + if (stat_result == 0 > + && (S_IFSOCK & statbuf.st_mode)) Don't think you need to break the line. You should use S_ISSOCK (statbuf.st_mode). > + unlink (sock_name); > > - if (listen_desc >= 0) > - break; > + addr = (struct sockaddr *) &unix_addr; > + addrlen = sizeof (unix_addr); > } > + else > + { > + struct addrinfo *iter; > > - if (iter == NULL) > - perror_with_name ("Can't open socket"); > + int r = getaddrinfo (parsed.host_str.c_str (), > + parsed.port_str.c_str (), > + &hint, &ainfo); > > - /* Allow rapid reuse of this port. */ > - tmp = 1; > - setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp, > - sizeof (tmp)); > + if (r != 0) > + error (_("%s: cannot resolve name: %s"), name, gai_strerror (r)); > > - 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); > + scoped_free_addrinfo freeaddrinfo (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 (listen_desc >= 0) > + break; > + } > + > + 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) > @@ -354,11 +412,11 @@ remote_open (const char *name) > } > #ifndef USE_WIN32API > else if (port_str == NULL) > - { > + { > struct stat statbuf; > > if (stat (name, &statbuf) == 0 > - && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode))) > + && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode))) These two changes are unrelated and should go in a separate patch (they can be pushed as obvious, BTW). > remote_desc = open (name, O_RDWR); > else > { > -- > 2.11.0 Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/