Remote debugging over local domain sockets?

Remote debugging over local domain sockets?

[John Darrington]

So far as I'm aware, it's not possible to run a remote target over
a local domain (AF_UNIX) socket.

Would you accept a patch which changes the behaviour of
"target remote /foo/bar"
such that if /foo/bar is a socket, gdb will connect to that
socket instead of trying to open it as a device?

J'

Re: Remote debugging over local domain sockets?

[Tom Tromey]

>>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:

John> So far as I'm aware, it's not possible to run a remote target over
John> a local domain (AF_UNIX) socket.

John> Would you accept a patch which changes the behaviour of "target
John> remote /foo/bar" such that if /foo/bar is a socket, gdb will
John> connect to that socket instead of trying to open it as a device?

How about stat'ing the path and doing the right thing depending on
whether it is a device or a socket?

Alternatively, how about new syntax instead?  "target remote /path" has meant
to open a serial device since forever.  However there's also syntax like
"target remote udp:host:port".  So maybe "target remote socket:/path"?

Tom

Re: Remote debugging over local domain sockets?

[John Darrington]

On Wed, Aug 29, 2018 at 09:17:00AM -0600, Tom Tromey wrote:
     >>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:
     
     John> So far as I'm aware, it's not possible to run a remote target over
     John> a local domain (AF_UNIX) socket.
     
     John> Would you accept a patch which changes the behaviour of "target
     John> remote /foo/bar" such that if /foo/bar is a socket, gdb will
     John> connect to that socket instead of trying to open it as a device?
     
     How about stat'ing the path and doing the right thing depending on
     whether it is a device or a socket?

That was my idea.
     
     Alternatively, how about new syntax instead?  "target remote /path" has meant
     to open a serial device since forever.  However there's also syntax like
     "target remote udp:host:port".  So maybe "target remote socket:/path"?
     
You mean where 'socket' is  the literal string "socket" ?  That might
also work, but one would have to consider what would happen if somebody
wanted to connect to a remote host called "socket" .   

I prefer the first idea.

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: Remote debugging over local domain sockets?

[Tom Tromey]

>>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:

>      How about stat'ing the path and doing the right thing depending on
>      whether it is a device or a socket?

John> That was my idea.
     
Hah, sorry.  I completely misread that sentence.

I think this would be fine.

Tom

[PATCH] Allow remote debugging over a local domain socket

[John Darrington]

Extend the "target remote"  and "target extended-remote" commands
such that if the filename provided is a unix domain (AF_UNIX)
socket, then it'll be treated as such, instead of trying to open
it as if it were a character device.

gdb/ChangeLog:
	* gdb/NEWS:  Mention changed commands.
	* gdb/configure.ac (SER_HARDWIRE): Add ser-socket.o
	* gdb/doc/gdb.texinfo (Remote Connection Commands):  Describe
          changed commands.
	* gdb/ser-socket.c: New file.
	* gdb/ser-socket.h: New file.
	* gdb/Makefile.in: Add new files.
	* gdb/serial.c (serial_open): Check if filename is a socket
          and lookup the appropriate interface accordingly.
---
 gdb/Makefile.in     |   2 +
 gdb/NEWS            |   5 ++
 gdb/configure.ac    |   2 +-
 gdb/doc/gdb.texinfo |  19 +++++++-
 gdb/ser-socket.c    | 132 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/ser-socket.h    |  31 ++++++++++++
 gdb/serial.c        |  10 +++-
 7 files changed, 198 insertions(+), 3 deletions(-)
 create mode 100644 gdb/ser-socket.c
 create mode 100644 gdb/ser-socket.h

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index f448d1ee19..cb98f987da 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1355,6 +1355,7 @@ HFILES_NO_SRCDIR = \
 	sentinel-frame.h \
 	ser-base.h \
 	ser-event.h \
+	ser-socket.h \
 	ser-tcp.h \
 	ser-unix.h \
 	serial.h \
@@ -2324,6 +2325,7 @@ ALLDEPFILES = \
 	ser-go32.c \
 	ser-mingw.c \
 	ser-pipe.c \
+	ser-socket.c \
 	ser-tcp.c \
 	sh-nbsd-nat.c \
 	sh-nbsd-tdep.c \
diff --git a/gdb/NEWS b/gdb/NEWS
index c46056525a..c7436e0ced 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -40,6 +40,11 @@ maint show dwarf unwinders
 
 * Changed commands
 
+target remote FILENAME
+target extended-remote FILENAME
+  If FILENAME is a unix domain socket gdb will attempt to connect
+  to this socket instead of opening FILENAME as a character device.
+
 thread apply [all | COUNT | -COUNT] [FLAG]... COMMAND
   The 'thread apply' command accepts new FLAG arguments.
   FLAG arguments allow to control what output to produce and how to handle
diff --git a/gdb/configure.ac b/gdb/configure.ac
index 13bc5f9a8f..f6735858d7 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1870,7 +1870,7 @@ lose
 
 
 dnl Figure out which of the many generic ser-*.c files the _host_ supports.
-SER_HARDWIRE="ser-base.o ser-unix.o ser-pipe.o ser-tcp.o"
+SER_HARDWIRE="ser-base.o ser-unix.o ser-pipe.o ser-socket.o ser-tcp.o"
 case ${host} in
   *go32* ) SER_HARDWIRE=ser-go32.o ;;
   *djgpp* ) SER_HARDWIRE=ser-go32.o ;;
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e4ecd57a9e..5ae53e0582 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -20703,7 +20703,8 @@ programs.
 
 @subsection Remote Connection Commands
 @cindex remote connection commands
-@value{GDBN} can communicate with the target over a serial line, or
+@value{GDBN} can communicate with the target over a serial line, a
+local socket, or
 over an @acronym{IP} network using @acronym{TCP} or @acronym{UDP}.  In
 each case, @value{GDBN} uses the same protocol for debugging your
 program; only the medium carrying the debugging packets varies.  The
@@ -20728,6 +20729,22 @@ If you're using a serial line, you may want to give @value{GDBN} the
 (@pxref{Remote Configuration, set serial baud}) before the
 @code{target} command.
 
+
+@item target remote @var{local-socket}
+@itemx target extended-remote @var{local-socket}
+@cindex local socket, @code{target remote}
+Use @var{local-socket} to communicate with the target.  For example,
+to use a local socket bound to the file system entry @file{/tmp/gdb-socket0}:
+
+@smallexample
+target remote /tmp/gdb-socket0
+@end smallexample
+
+Note that this command has the same form as the command to connect
+to a serial line.  @value{GDBN} will automatically determine which
+kind of file you have specified and will make the appropriate kind
+of connection.
+
 @item target remote @code{@var{host}:@var{port}}
 @itemx target remote @code{@var{[host]}:@var{port}}
 @itemx target remote @code{tcp:@var{host}:@var{port}}
diff --git a/gdb/ser-socket.c b/gdb/ser-socket.c
new file mode 100644
index 0000000000..54d14f36ad
--- /dev/null
+++ b/gdb/ser-socket.c
@@ -0,0 +1,132 @@
+/* Serial interface for local domain connections on Un*x like systems.
+
+   Copyright (C) 1992-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "serial.h"
+#include "ser-base.h"
+#include "ser-socket.h"
+
+#include <sys/socket.h>
+#include <sys/un.h>
+
+
+/* Open a AF_UNIX socket.  */
+int
+socket_open (struct serial *scb, const char *name)
+{
+  struct sockaddr_un addr;
+
+  memset (&addr, 0, sizeof addr);
+  addr.sun_family = AF_UNIX;
+#ifndef UNIX_MAX_PATH
+# define UNIX_MAX_PATH 108
+#endif
+  strncpy (addr.sun_path, name, UNIX_MAX_PATH - 1);
+
+  int sock = socket (AF_UNIX, SOCK_STREAM, 0);
+
+  if (connect (sock, (struct sockaddr *) &addr,
+	       sizeof (struct sockaddr_un)) < 0)
+    {
+      close (sock);
+      scb->fd = -1;
+      return -1;
+    }
+
+  scb->fd = sock;
+
+  return 0;
+}
+
+void
+socket_close (struct serial *scb)
+{
+  if (scb->fd == -1)
+    return;
+
+  close (scb->fd);
+  scb->fd = -1;
+}
+
+int
+socket_read_prim (struct serial *scb, size_t count)
+{
+  /* Need to cast to silence -Wpointer-sign on MinGW, as Winsock's
+     'recv' takes 'char *' as second argument, while 'scb->buf' is
+     'unsigned char *'.  */
+  return recv (scb->fd, (char *) scb->buf, count, 0);
+}
+
+int
+socket_write_prim (struct serial *scb, const void *buf, size_t count)
+{
+  /* On Windows, the second parameter to send is a "const char *"; on
+     UNIX systems it is generally "const void *".  The cast to "const
+     char *" is OK everywhere, since in C++ any data pointer type can
+     be implicitly converted to "const void *".  */
+  return send (scb->fd, (const char *) buf, count, 0);
+}
+
+int
+ser_socket_send_break (struct serial *scb)
+{
+  /* Send telnet IAC and BREAK characters.  */
+  return (serial_write (scb, "\377\363", 2));
+}
+
+#ifndef USE_WIN32API
+
+/* The SOCKET ops.  */
+
+static const struct serial_ops socket_ops =
+{
+  "socket",
+  socket_open,
+  socket_close,
+  NULL,
+  ser_base_readchar,
+  ser_base_write,
+  ser_base_flush_output,
+  ser_base_flush_input,
+  ser_socket_send_break,
+  ser_base_raw,
+  ser_base_get_tty_state,
+  ser_base_copy_tty_state,
+  ser_base_set_tty_state,
+  ser_base_print_tty_state,
+  ser_base_setbaudrate,
+  ser_base_setstopbits,
+  ser_base_setparity,
+  ser_base_drain_output,
+  ser_base_async,
+  socket_read_prim,
+  socket_write_prim
+};
+
+#endif /* USE_WIN32API */
+
+void
+_initialize_ser_socket (void)
+{
+#ifdef USE_WIN32API
+  /* Do nothing; Windoze does not have local domain sockets. */
+#else
+  serial_add_interface (&socket_ops);
+#endif /* USE_WIN32API */
+}
diff --git a/gdb/ser-socket.h b/gdb/ser-socket.h
new file mode 100644
index 0000000000..58509302d6
--- /dev/null
+++ b/gdb/ser-socket.h
@@ -0,0 +1,31 @@
+/* Serial interface for raw TCP connections on Un*x like systems.
+
+   Copyright (C) 2006-2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef SER_SOCKET_H
+#define SER_SOCKET_H
+
+struct serial;
+
+extern int socket_open (struct serial *scb, const char *name);
+extern void socket_close (struct serial *scb);
+extern int socket_read_prim (struct serial *scb, size_t count);
+extern int socket_write_prim (struct serial *scb, const void *buf, size_t count);
+extern int ser_socket_send_break (struct serial *scb);
+
+#endif
diff --git a/gdb/serial.c b/gdb/serial.c
index fb2b212918..13b1af3873 100644
--- a/gdb/serial.c
+++ b/gdb/serial.c
@@ -213,7 +213,15 @@ serial_open (const char *name)
   else if (strchr (name, ':'))
     ops = serial_interface_lookup ("tcp");
   else
-    ops = serial_interface_lookup ("hardwire");
+    {
+      /* Check to see if name is a socket.  If it is, then treat is
+         as such.  Otherwise assume that it's a character device.  */
+      struct stat sb;
+      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))
+	ops = serial_interface_lookup ("socket");
+      else
+	ops = serial_interface_lookup ("hardware");
+    }
 
   if (!ops)
     return NULL;
-- 
2.11.0

Re: [PATCH] Allow remote debugging over a local domain socket

[Eli Zaretskii]

> From: John Darrington <john@darrington.wattle.id.au>
> Cc: John Darrington <john@darrington.wattle.id.au>
> Date: Fri, 31 Aug 2018 12:18:18 +0200
> 
> Extend the "target remote"  and "target extended-remote" commands
> such that if the filename provided is a unix domain (AF_UNIX)
> socket, then it'll be treated as such, instead of trying to open
> it as if it were a character device.

Thanks.

> +target remote FILENAME
> +target extended-remote FILENAME
> +  If FILENAME is a unix domain socket gdb will attempt to connect
                                        ^
Missing comma.  Also, I believe "unix" should be "Unix" and "gdb"
should be "GDB".

>  @subsection Remote Connection Commands
>  @cindex remote connection commands
> -@value{GDBN} can communicate with the target over a serial line, or
> +@value{GDBN} can communicate with the target over a serial line, a
> +local socket, or

I'd prefer to say "local Unix domain socket" here.

> +@item target remote @var{local-socket}
> +@itemx target extended-remote @var{local-socket}
> +@cindex local socket, @code{target remote}
> +Use @var{local-socket} to communicate with the target.  For example,
> +to use a local socket bound to the file system entry @file{/tmp/gdb-socket0}:

Likewise here.  Alternatively, add a note that this only works on
systems that support Unix domain sockets.

The documentation parts are approved with these fixed.

> +int
> +socket_read_prim (struct serial *scb, size_t count)
> +{
> +  /* Need to cast to silence -Wpointer-sign on MinGW, as Winsock's
> +     'recv' takes 'char *' as second argument, while 'scb->buf' is
> +     'unsigned char *'.  */
> +  return recv (scb->fd, (char *) scb->buf, count, 0);
> +}
> +
> +int
> +socket_write_prim (struct serial *scb, const void *buf, size_t count)
> +{
> +  /* On Windows, the second parameter to send is a "const char *"; on
> +     UNIX systems it is generally "const void *".  The cast to "const
> +     char *" is OK everywhere, since in C++ any data pointer type can
> +     be implicitly converted to "const void *".  */
> +  return send (scb->fd, (const char *) buf, count, 0);
> +}

I'm confused: why does this mention MinGW and Windows, when Windows
doesn't support AF_UNIX (AFAIK)?  Should this stuff even be compiled
on Windows?

> +#ifdef USE_WIN32API
> +  /* Do nothing; Windoze does not have local domain sockets. */

Exactly!

> -    ops = serial_interface_lookup ("hardwire");
> +    {
> +      /* Check to see if name is a socket.  If it is, then treat is
> +         as such.  Otherwise assume that it's a character device.  */
> +      struct stat sb;
> +      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))

AFAIK, S_IFSOCK is not defined in the MinGW headers, so we need some
replacement definition, or we need to ifdef this away in the Windows
build.

Thanks.

Re: [PATCH] Allow remote debugging over a local domain socket

[John Darrington]

On Fri, Aug 31, 2018 at 05:58:47PM +0300, Eli Zaretskii wrote:

     > +int
     > +socket_read_prim (struct serial *scb, size_t count)
     > +{
     > +  /* Need to cast to silence -Wpointer-sign on MinGW, as Winsock's
     > +     'recv' takes 'char *' as second argument, while 'scb->buf' is
     > +     'unsigned char *'.  */
     > +  return recv (scb->fd, (char *) scb->buf, count, 0);
     > +}
     > +
     > +int
     > +socket_write_prim (struct serial *scb, const void *buf, size_t count)
     > +{
     > +  /* On Windows, the second parameter to send is a "const char *"; on
     > +     UNIX systems it is generally "const void *".  The cast to "const
     > +     char *" is OK everywhere, since in C++ any data pointer type can
     > +     be implicitly converted to "const void *".  */
     > +  return send (scb->fd, (const char *) buf, count, 0);
     > +}
     
     I'm confused: why does this mention MinGW and Windows, when Windows
     doesn't support AF_UNIX (AFAIK)?  Should this stuff even be compiled
     on Windows?
     
     > +#ifdef USE_WIN32API
     > +  /* Do nothing; Windoze does not have local domain sockets. */
     
     Exactly!

     
     > -    ops = serial_interface_lookup ("hardwire");
     > +    {
     > +      /* Check to see if name is a socket.  If it is, then treat is
     > +         as such.  Otherwise assume that it's a character device.  */
     > +      struct stat sb;
     > +      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))
     
     AFAIK, S_IFSOCK is not defined in the MinGW headers, so we need some
     replacement definition, or we need to ifdef this away in the Windows
     build.

I am told, that the recent versions of Windows 10 do indeed have local
domain sockets.    But if you like, I can conditionally include the file
in the SER_HARDWIRE variable.  Then the whole issue is moot. 

J'

     

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH] Allow remote debugging over a local domain socket

[Eli Zaretskii]

> Date: Fri, 31 Aug 2018 17:10:38 +0200
> From: John Darrington <john@darrington.wattle.id.au>
> Cc: John Darrington <john@darrington.wattle.id.au>, gdb-patches@sourceware.org
> 
>      AFAIK, S_IFSOCK is not defined in the MinGW headers, so we need some
>      replacement definition, or we need to ifdef this away in the Windows
>      build.
> 
> I am told, that the recent versions of Windows 10 do indeed have local
> domain sockets.

That's OK, but even the newer versions of MinGW don't yet have
S_IFSOCK in their headers, AFAICS, so they will fail to compile this
code.

Re: [PATCH] Allow remote debugging over a local domain socket

[Tom Tromey]

>>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:

John> Extend the "target remote"  and "target extended-remote" commands
John> such that if the filename provided is a unix domain (AF_UNIX)
John> socket, then it'll be treated as such, instead of trying to open
John> it as if it were a character device.

Thanks for the patch.
This looks essentially reasonable to me.

John> +/* Open a AF_UNIX socket.  */
John> +int
John> +socket_open (struct serial *scb, const char *name)
John> +{

It seems to me that all the functions in this file could be static.
This might necessitate wrapping many of them in "#ifndef USE_WIN32API"
to avoid warnings about unused code, but that seems like an improvement
as well.

John> +int
John> +ser_socket_send_break (struct serial *scb)
John> +{
John> +  /* Send telnet IAC and BREAK characters.  */
John> +  return (serial_write (scb, "\377\363", 2));
John> +}

I don't really know what's expected here, but is this correct?

John> diff --git a/gdb/ser-socket.h b/gdb/ser-socket.h
John> new file mode 100644
John> index 0000000000..58509302d6
John> --- /dev/null
John> +++ b/gdb/ser-socket.h

You could just drop this file entirely.

Tom

Re: [PATCH] Allow remote debugging over a local domain socket

[John Darrington]

On Fri, Aug 31, 2018 at 09:09:23AM -0600, Tom Tromey wrote:
     >>>>> "John" == John Darrington <john@darrington.wattle.id.au> writes:
     
     John> Extend the "target remote"  and "target extended-remote" commands
     John> such that if the filename provided is a unix domain (AF_UNIX)
     John> socket, then it'll be treated as such, instead of trying to open
     John> it as if it were a character device.
     
     Thanks for the patch.
     This looks essentially reasonable to me.
Thanks
     
     John> +/* Open a AF_UNIX socket.  */
     John> +int
     John> +socket_open (struct serial *scb, const char *name)
     John> +{
     
     It seems to me that all the functions in this file could be static.
     This might necessitate wrapping many of them in "#ifndef USE_WIN32API"
     to avoid warnings about unused code, but that seems like an improvement
     as well.

OK
     
     John> +int
     John> +ser_socket_send_break (struct serial *scb)
     John> +{
     John> +  /* Send telnet IAC and BREAK characters.  */
     John> +  return (serial_write (scb, "\377\363", 2));
     John> +}
     
     I don't really know what's expected here, but is this correct?

I wondererd about that too.  This bit I copied from the ser-tcp.c file.
     
     John> diff --git a/gdb/ser-socket.h b/gdb/ser-socket.h
     John> new file mode 100644
     John> index 0000000000..58509302d6
     John> --- /dev/null
     John> +++ b/gdb/ser-socket.h
     
     You could just drop this file entirely.
     
OK.

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH] Allow remote debugging over a local domain socket

[Pedro Alves]

On 08/31/2018 11:18 AM, John Darrington wrote:
> Extend the "target remote"  and "target extended-remote" commands
> such that if the filename provided is a unix domain (AF_UNIX)
> socket, then it'll be treated as such, instead of trying to open
> it as if it were a character device.

What server are you using this against?  It'd be great to add
support for gdbserver as well.  Were you planning on doing it?
If we had that, then I think we could add unix domain socket testing
to gdb/testsuite/gdb.server/server-connect.exp (assuming it'd be easy
enough to create a socket from tcl).

> 
> gdb/ChangeLog:
> 	* gdb/NEWS:  Mention changed commands.
> 	* gdb/configure.ac (SER_HARDWIRE): Add ser-socket.o
> 	* gdb/ser-socket.c: New file.
> 	* gdb/ser-socket.h: New file.
> 	* gdb/Makefile.in: Add new files.
> 	* gdb/serial.c (serial_open): Check if filename is a socket
>           and lookup the appropriate interface accordingly.

Remove leading "gdb/" from all these entries.  The paths in the
entry are relative to the ChangeLog file.

> 	* gdb/doc/gdb.texinfo (Remote Connection Commands):  Describe
>           changed commands.

gdb/doc/ has its own ChangeLog file.  Move it there, and remove the
"gdb/doc/" prefix.  Update "Describe changed commands" to be a bit
more standalone, since it won't have the gdb/ changes context at
hand.

Also please fix the double-space, missing period, and check tab vs spaces
in the entry.


> +
> +
> +/* Open a AF_UNIX socket.  */
> +int
> +socket_open (struct serial *scb, const char *name)
> +{
> +  struct sockaddr_un addr;
> +
> +  memset (&addr, 0, sizeof addr);
> +  addr.sun_family = AF_UNIX;
> +#ifndef UNIX_MAX_PATH
> +# define UNIX_MAX_PATH 108
> +#endif

gdbserver/tracepoint.c does:

#ifndef UNIX_PATH_MAX
#define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
#endif

> diff --git a/gdb/serial.c b/gdb/serial.c
> index fb2b212918..13b1af3873 100644
> --- a/gdb/serial.c
> +++ b/gdb/serial.c
> @@ -213,7 +213,15 @@ serial_open (const char *name)
>    else if (strchr (name, ':'))
>      ops = serial_interface_lookup ("tcp");
>    else
> -    ops = serial_interface_lookup ("hardwire");
> +    {
> +      /* Check to see if name is a socket.  If it is, then treat is
> +         as such.  Otherwise assume that it's a character device.  */
> +      struct stat sb;
> +      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))
> +	ops = serial_interface_lookup ("socket");
> +      else
> +	ops = serial_interface_lookup ("hardware");


Nit: maybe it's just me, but I find "socket" ambiguous -- is it
a unix domain socket, a tcp socket, a udp socket, other?
I'd have picked "unix" or "uds" instead, and likewise have
named the file ser-unix.c and functions unix_foo instead
of serial.  We already have ser-unix.c, but since this is
only for unix really, then we could add the new code in
that file instead?

Thanks,
Pedro Alves

Re: [PATCH] Allow remote debugging over a local domain socket

[John Darrington]

On Fri, Aug 31, 2018 at 05:00:55PM +0100, Pedro Alves wrote:
     
     What server are you using this against?  It'd be great to add
     support for gdbserver as well.  Were you planning on doing it?

I'm using upad [1], but a version which has not yet been released (the
released one doesn't have the necessary features).
I wasn't planning to update gdbserver but I could do that when ...

     If we had that, then I think we could add unix domain socket testing
     to gdb/testsuite/gdb.server/server-connect.exp (assuming it'd be easy
     enough to create a socket from tcl).

Yes.  One of the big advantages of using local sockets in testing as
opposed to tcp sockets is that parallel tests become a lot easier.  You
don't have to worry about port number conflicts or wait times.

However to do it right is not altogether straightforward.  You'd need
gdbserver to have some kind of daemon mode, for example

tempdir=$(mkdir -d)
gdbserver --socket=$tempdir/mysock --start
gdb --iex="target remote $tempdir/mysock" ...
gdbserver --socket=$tempdir/mysock --stop
rm -rf $tempdir

This is because there is a race condition in a server between the 
bind and listen syscalls.  GDB must not attempt to connect until
listen has returned successfully.


[1] http://www.nongnu.org/micropad
     
     
     gdbserver/tracepoint.c does:
     
     #ifndef UNIX_PATH_MAX
     #define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
     #endif

I'm not sure if that's entirely safe.  I believe some systems define
sun_path as a pointer into a static buffer in the kernel.  But if some
higher authority can say otherwise I'll defer to them.

     > diff --git a/gdb/serial.c b/gdb/serial.c
     > index fb2b212918..13b1af3873 100644
     > --- a/gdb/serial.c
     > +++ b/gdb/serial.c
     > @@ -213,7 +213,15 @@ serial_open (const char *name)
     >    else if (strchr (name, ':'))
     >      ops = serial_interface_lookup ("tcp");
     >    else
     > -    ops = serial_interface_lookup ("hardwire");
     > +    {
     > +      /* Check to see if name is a socket.  If it is, then treat is
     > +         as such.  Otherwise assume that it's a character device.  */
     > +      struct stat sb;
     > +      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))
     > +	ops = serial_interface_lookup ("socket");
     > +      else
     > +	ops = serial_interface_lookup ("hardware");
     
     
     Nit: maybe it's just me, but I find "socket" ambiguous -- is it
     a unix domain socket, a tcp socket, a udp socket, other?
     I'd have picked "unix" or "uds" instead, and likewise have
     named the file ser-unix.c and functions unix_foo instead
     of serial.  We already have ser-unix.c, but since this is
     only for unix really, then we could add the new code in
     that file instead?

Let's face it, the names of these files are totally anachronistic.
ser-tcp.c has nothing to do with serial ports and serial.c does only
tangentially.

It could use a big rename action ...

J'
     

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH] Allow remote debugging over a local domain socket

[Pedro Alves]

On 08/31/2018 05:40 PM, John Darrington wrote:
> On Fri, Aug 31, 2018 at 05:00:55PM +0100, Pedro Alves wrote:
>      
>      What server are you using this against?  It'd be great to add
>      support for gdbserver as well.  Were you planning on doing it?
> 
> I'm using upad [1], but a version which has not yet been released (the
> released one doesn't have the necessary features).
> I wasn't planning to update gdbserver but I could do that when ...
> 
>      If we had that, then I think we could add unix domain socket testing
>      to gdb/testsuite/gdb.server/server-connect.exp (assuming it'd be easy
>      enough to create a socket from tcl).
> 
> Yes.  One of the big advantages of using local sockets in testing as
> opposed to tcp sockets is that parallel tests become a lot easier.  

That's not what I suggested.  The server-connect.exp test does this:

 # Test multiple types of connection (IPv4, IPv6, TCP, UDP) and make
 # sure both gdbserver and GDB work.

so what I meant was that we could add unix socket testing to that file
in order to smoke test unix socket work and continue working, regardless
of how the rest of the testsuite is being run.

> You don't have to worry about port number conflicts or wait times.
> 
> However to do it right is not altogether straightforward.  You'd need
> gdbserver to have some kind of daemon mode, for example
> 
> tempdir=$(mkdir -d)
> gdbserver --socket=$tempdir/mysock --start
> gdb --iex="target remote $tempdir/mysock" ...
> gdbserver --socket=$tempdir/mysock --stop
> rm -rf $tempdir
> 
> This is because there is a race condition in a server between the 
> bind and listen syscalls.  GDB must not attempt to connect until
> listen has returned successfully.

Can you clarify how unix sockets are different from tcp sockets here?

> 
> 
> [1] http://www.nongnu.org/micropad
>      
>      
>      gdbserver/tracepoint.c does:
>      
>      #ifndef UNIX_PATH_MAX
>      #define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
>      #endif
> 
> I'm not sure if that's entirely safe.  I believe some systems define
> sun_path as a pointer into a static buffer in the kernel.  

How can userspace have a pointer into kernel memory?

> But if some higher authority can say otherwise I'll defer to them.

What do you mean by "higher authority"?

If you google around a bit, you find references to kernels other than
Linux having a limit different than 108.  E.g., from:

 https://unix.stackexchange.com/questions/367008/why-is-socket-path-length-limited-to-a-hundred-chars

    OpenBSD: 104 characters
    FreeBSD: 104 characters
    Mac OS X 10.9: 104 characters

So hardcoding to 108 seems worse to me.

Regardless, seems odd to have different parts of gdb use different
fallbacks.  Ideally, we'd move the fallback definition to a single
place used by both users. 

> 
>      > diff --git a/gdb/serial.c b/gdb/serial.c
>      > index fb2b212918..13b1af3873 100644
>      > --- a/gdb/serial.c
>      > +++ b/gdb/serial.c
>      > @@ -213,7 +213,15 @@ serial_open (const char *name)
>      >    else if (strchr (name, ':'))
>      >      ops = serial_interface_lookup ("tcp");
>      >    else
>      > -    ops = serial_interface_lookup ("hardwire");
>      > +    {
>      > +      /* Check to see if name is a socket.  If it is, then treat is
>      > +         as such.  Otherwise assume that it's a character device.  */
>      > +      struct stat sb;
>      > +      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))
>      > +	ops = serial_interface_lookup ("socket");
>      > +      else
>      > +	ops = serial_interface_lookup ("hardware");
>      
>      
>      Nit: maybe it's just me, but I find "socket" ambiguous -- is it
>      a unix domain socket, a tcp socket, a udp socket, other?
>      I'd have picked "unix" or "uds" instead, and likewise have
>      named the file ser-unix.c and functions unix_foo instead
>      of serial.  We already have ser-unix.c, but since this is
>      only for unix really, then we could add the new code in
>      that file instead?
> 
> Let's face it, the names of these files are totally anachronistic.
> ser-tcp.c has nothing to do with serial ports and serial.c does only
> tangentially.

All these files provide different implementations of serial transports
(as opposed to parallel), abstracted behind "struct serial", and to
be used with the "remote SERIAL protocol".  It's not that tangential.

We have three host-specific files named such that their name indicates
the host which they are for:

 "ser-unix.c", "ser-go32.c" and "ser-mingw.c".

Then we have host-independent files that are named wrt to the
transport they implement internally:

 "ser-event.c", "ser-pipe.c", "ser-tcp.c".

ser-event.c is a bit of an outlier, if you'd like to
pick on some case.

> It could use a big rename action ...

Sure, it could be better.  

But, is "socket" your ideal choice?

Thanks,
Pedro Alves

Re: [PATCH] Allow remote debugging over a local domain socket

[John Darrington]

Hi Pedro,

On Mon, Sep 03, 2018 at 02:18:55PM +0100, Pedro Alves wrote:

     > Yes.  One of the big advantages of using local sockets in testing as
     > opposed to tcp sockets is that parallel tests become a lot easier.  
     
     That's not what I suggested.  The server-connect.exp test does this:
     
      # Test multiple types of connection (IPv4, IPv6, TCP, UDP) and make
      # sure both gdbserver and GDB work.
     
     so what I meant was that we could add unix socket testing to that file
     in order to smoke test unix socket work and continue working, regardless
     of how the rest of the testsuite is being run.

Oh right.  Yes that could be done, but like you said it would involve 
modifying gdbserver.

     Can you clarify how unix sockets are different from tcp sockets here?

1.  Unix sockets have a (almost) infinite choice of connection points.
Whereas TCP sockets you're limited by the port number (usually 16 bits).

2.  One can never be sure that a TCP port doesn't already have something
listening on it.  So starting a server cannot be guaranteed to succeed.
Whereas with a Unix socket you can create the directory where the file
entry is to reside and know it'll be empty.

3.  If a server listening on a TCP socket crashes, then that port number
will be unavailable until the kernel's timeout expires (usually after ~
2 minutes).  You cannot restart the server until that happens.   There
is no way (outside of unloading the kernel's TCP/IP stack) to force the
port to become a available again.  With Unix sockets, you simply unlink
the filename.

4.  Unix sockets can only be used for communication between processes on
the same host.
     
     
     > 
     > I'm not sure if that's entirely safe.  I believe some systems define
     > sun_path as a pointer into a static buffer in the kernel.  
     
     How can userspace have a pointer into kernel memory?

I recall from Stevens or somewhere that some systems used to 
define struct socket_un like 
{
  int sun_family;
  char *sun_path;
}

or something.   I don't remember the details of which system it was or
how it worked.
     
     
         OpenBSD: 104 characters
         FreeBSD: 104 characters
         Mac OS X 10.9: 104 characters
     
     So hardcoding to 108 seems worse to me.

I thought posix required a minimum of 108.
     
     Regardless, seems odd to have different parts of gdb use different
     fallbacks.  Ideally, we'd move the fallback definition to a single
     place used by both users. 

I don't mind. If you want me to copy the macro from there, I can do
that.
     
     All these files provide different implementations of serial transports
     (as opposed to parallel), abstracted behind "struct serial", and to
     be used with the "remote SERIAL protocol".  It's not that tangential.
     
     We have three host-specific files named such that their name indicates
     the host which they are for:
     
      "ser-unix.c", "ser-go32.c" and "ser-mingw.c".
     
     Then we have host-independent files that are named wrt to the
     transport they implement internally:
     
      "ser-event.c", "ser-pipe.c", "ser-tcp.c".
     
     ser-event.c is a bit of an outlier, if you'd like to
     pick on some case.
     
     > It could use a big rename action ...
     
     Sure, it could be better.  
     
     But, is "socket" your ideal choice?

My first choice was ser-unix.c but that is already taken.   I really
don't have a preference.  What would you prefer?  ser-local.c perhaps?

     
Thanks for the review.

J'

Re: [PATCH] Allow remote debugging over a local domain socket

[Pedro Alves]

On 09/03/2018 07:48 PM, John Darrington wrote:
> Hi Pedro,
> 
> On Mon, Sep 03, 2018 at 02:18:55PM +0100, Pedro Alves wrote:
> 
>      > Yes.  One of the big advantages of using local sockets in testing as
>      > opposed to tcp sockets is that parallel tests become a lot easier.  
>      
>      That's not what I suggested.  The server-connect.exp test does this:
>      
>       # Test multiple types of connection (IPv4, IPv6, TCP, UDP) and make
>       # sure both gdbserver and GDB work.
>      
>      so what I meant was that we could add unix socket testing to that file
>      in order to smoke test unix socket work and continue working, regardless
>      of how the rest of the testsuite is being run.
> 
> Oh right.  Yes that could be done, but like you said it would involve 
> modifying gdbserver.
> 
>      Can you clarify how unix sockets are different from tcp sockets here?
> 
> 1.  Unix sockets have a (almost) infinite choice of connection points.
> Whereas TCP sockets you're limited by the port number (usually 16 bits).
> 
> 2.  One can never be sure that a TCP port doesn't already have something
> listening on it.  So starting a server cannot be guaranteed to succeed.
> Whereas with a Unix socket you can create the directory where the file
> entry is to reside and know it'll be empty.
> 
> 3.  If a server listening on a TCP socket crashes, then that port number
> will be unavailable until the kernel's timeout expires (usually after ~
> 2 minutes).  You cannot restart the server until that happens.   There
> is no way (outside of unloading the kernel's TCP/IP stack) to force the
> port to become a available again.  With Unix sockets, you simply unlink
> the filename.
> 
> 4.  Unix sockets can only be used for communication between processes on
> the same host.

What I meant with "different ... here" was, how are unix domain sockets
different from tcp sockets with respect to:

 "This is because there is a race condition in a server between the 
 bind and listen syscalls.  GDB must not attempt to connect until
 listen has returned successfully."

If GDB attempts to connect to a tcp gdbserver before it listens/binds,
then the connection will fail too.  Except, GDB retries the
connection, but that would be the case with unix domain sockets
too, no?

Re. your point 3 above, I don't observe that with gdbserver, maybe
because it enables fast reuse with SO_REUSEADDR.

>      
>      
>      > 
>      > I'm not sure if that's entirely safe.  I believe some systems define
>      > sun_path as a pointer into a static buffer in the kernel.  
>      
>      How can userspace have a pointer into kernel memory?
> 
> I recall from Stevens or somewhere that some systems used to 
> define struct socket_un like 
> {
>   int sun_family;
>   char *sun_path;
> }
> 
> or something.   I don't remember the details of which system it was or
> how it worked.
>      
>      
>          OpenBSD: 104 characters
>          FreeBSD: 104 characters
>          Mac OS X 10.9: 104 characters
>      
>      So hardcoding to 108 seems worse to me.
> 
> I thought posix required a minimum of 108.
>      
>      Regardless, seems odd to have different parts of gdb use different
>      fallbacks.  Ideally, we'd move the fallback definition to a single
>      place used by both users. 
> 
> I don't mind. If you want me to copy the macro from there, I can do
> that.

Please do.

>      
>      All these files provide different implementations of serial transports
>      (as opposed to parallel), abstracted behind "struct serial", and to
>      be used with the "remote SERIAL protocol".  It's not that tangential.
>      
>      We have three host-specific files named such that their name indicates
>      the host which they are for:
>      
>       "ser-unix.c", "ser-go32.c" and "ser-mingw.c".
>      
>      Then we have host-independent files that are named wrt to the
>      transport they implement internally:
>      
>       "ser-event.c", "ser-pipe.c", "ser-tcp.c".
>      
>      ser-event.c is a bit of an outlier, if you'd like to
>      pick on some case.
>      
>      > It could use a big rename action ...
>      
>      Sure, it could be better.  
>      
>      But, is "socket" your ideal choice?
> 
> My first choice was ser-unix.c but that is already taken.   I really
> don't have a preference.  What would you prefer?  ser-local.c perhaps?
> 

As I had mentioned before, if "unix" is taken, I'd prefer "ser-uds.c", for
"Unix Domain Socket", and uds_ as function/symbol prefix.  I think UDS
is quite established and clearer than "local".  I get where it comes
from, but note how "local" isn't even ever mentioned anywhere in
 <https://en.wikipedia.org/wiki/Unix_domain_socket>,
for example.

I'll take a look at your v3 patch, see if I have extra comments.

Thanks,
Pedro Alves

Re: [PATCH] Allow remote debugging over a local domain socket

[John Darrington]

Hi Pedro, 

Nice to hear from you.

On Mon, Oct 01, 2018 at 08:45:52PM +0100, Pedro Alves wrote:

     >      Can you clarify how unix sockets are different from tcp sockets here?
     > 
     > 1.  Unix sockets have a (almost) infinite choice of connection points.
     > Whereas TCP sockets you're limited by the port number (usually 16 bits).
     > 
     > 2.  One can never be sure that a TCP port doesn't already have something
     > listening on it.  So starting a server cannot be guaranteed to succeed.
     > Whereas with a Unix socket you can create the directory where the file
     > entry is to reside and know it'll be empty.
     > 
     > 3.  If a server listening on a TCP socket crashes, then that port number
     > will be unavailable until the kernel's timeout expires (usually after ~
     > 2 minutes).  You cannot restart the server until that happens.   There
     > is no way (outside of unloading the kernel's TCP/IP stack) to force the
     > port to become a available again.  With Unix sockets, you simply unlink
     > the filename.
     > 
     > 4.  Unix sockets can only be used for communication between processes on
     > the same host.
     
     What I meant with "different ... here" was, how are unix domain sockets
     different from tcp sockets with respect to:
     
      "This is because there is a race condition in a server between the 
      bind and listen syscalls.  GDB must not attempt to connect until
      listen has returned successfully."
     
     If GDB attempts to connect to a tcp gdbserver before it listens/binds,
     then the connection will fail too.  Except, GDB retries the
     connection, but that would be the case with unix domain sockets
     too, no?

You are right.  There is no difference in this respect.  My only reason
for mentioning it was that it is the only problem affecting concurrent
gdb/gdbserver sessions which is *not* solved by the use of unix domain
sockets.
     
     > 
     > I don't mind. If you want me to copy the macro from there, I can do
     > that.
     
     Please do.

Done.
     
     > My first choice was ser-unix.c but that is already taken.   I really
     > don't have a preference.  What would you prefer?  ser-local.c perhaps?
     > 
     
     As I had mentioned before, if "unix" is taken, I'd prefer "ser-uds.c", for
     "Unix Domain Socket", and uds_ as function/symbol prefix.  I think UDS
     is quite established and clearer than "local".  I get where it comes
     from, but note how "local" isn't even ever mentioned anywhere in
      <https://en.wikipedia.org/wiki/Unix_domain_socket>,
     for example.

I don't remember you suggesting ser-uds.c before. But no problem - I
will make that change.

     
Thanks for your comments.  I will attend to them all tonight and check
it in.

J'