Re: [PATCH] Implement IPv6 support for GDB/gdbserver

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, June 06 2018, Pedro Alves wrote:

> Hi Sergio,

Hi Pedro,

Thanks for the review.

> I noticed this when applying the patch:
>
>  $ git am /tmp/sergio.mbox
>  Applying: Implement IPv6 support for GDB/gdbserver
>  .git/rebase-apply/patch:982: trailing whitespace.
>            do 
>  .git/rebase-apply/patch:985: trailing whitespace.
>              } 
>  warning: 2 lines add whitespace errors.
>
> You can check it locally with:
>
>  $ git show --check
>
>  gdb/ser-tcp.c:256: trailing whitespace.
>  +         do 
>  gdb/ser-tcp.c:259: trailing whitespace.
>  +           } 
>
> Comments on the patch below.

Yeah, I just reindented this region, without touching anything else.
I've now removed these trailing whitespaces.

>> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
>> parameter, which instructs GDB and gdbserver to use IPv6 for
>> connections.  This way, if you want to run IPv6 tests, you do:
>> 
>>   $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'
>
> That sounds useful, but:
>
> #1 - I don't see how that works without also passing
>      --target_board= pointing at one of the native-gdbserver and
>      native-extended-gdbserver board files.  
>      Can you expand on why you took this approach instead of:
>  
>   a) handling GDB_TEST_IPV6 somewhere central, like
>      in gdb/testsuite/gdbserver-support.exp, where we
>      default to "localhost:".  That would exercise the gdb.server/
>      tests with ipv6, when testing with the default/unix board file.
>
>   b) add new board files to test with ipv6, like native-gdbserver-v6
>      or something like that.
>
>   c) both?

I was thinking about a good way to test this feature, and my initial
assumption was that the test would only make sense when --target-board=
is passed.  That's why I chose to implement the mechanism on
gdb/testsuite/boards/gdbserver-base.exp.  Now that you mentioned this, I
noticed that I should have also mentioned these expectations while
writing the commit message, and that the "make check-gdb
RUNTESTFLAGS='GDB_TEST_IPV6=1'" is actually wrong because it doesn't
specify any of the target boards.

Having said that, and after reading your question, I understand that the
testing can be made more flexible by implementing the logic inside
gdb/testsuite/gdbserver-support.exp instead, which will have the benefit
of activating the test even without a gdbserver target board being
specified.  I will give it a try and see if I can implement it in a
better way.

> #2 - I think it'd be also useful to have some gdb.server/ test that
>      runs with the default board and exercises / smoke tests ipv6.
>      (And if we have that, we might as well iterate the test on udp/udpv6
>      too.)

I thought about that, but I didn't know how to proceed when you don't
really know whether IPv6 support is present or not in the machine.  I'll
give it another try.

> #3 - Actually, this makes me wonder about changing the variable's
>      spelling from GDB_TEST_IPV6=1 to something like
>      GDB_TEST_SOCKETHOST and then one would be able to set it to:
>
>       "localhost:",
>       "localhost6:"
>       "tcp:localhost6:"
>       "\[::1\]:"
>       "udp:127.0.0.1:"
>
>      or whatever one would like.

That works, and has the benefit of being protocol-agnostic.  I'll
implement it.

> #4 - Why do we need to override get_comm_port too, here? :
>
>      -set_board_info sockethost "localhost:"
>      +if { [info exists GDB_TEST_IPV6] } {
>      +    set_board_info sockethost "tcp6:\[::1\]:"
>      +    set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
>      +} else {
>      +    set_board_info sockethost "localhost:"
>      +}
>
>     Doesn't overriding "sockethost" alone work?  Why not?

Hm.  I can swear this wasn't working before (otherwise I wouldn't have
created more work just by unnecessarily implement this override), but
now that I commented it out, it's working again.  This is
frustrating...  Anyway, I'm going to rewrite a lot of this code, so this
part will disappear.

>>  gdb/Makefile.in                                    |   2 +
>>  gdb/NEWS                                           |   4 +
>>  gdb/common/netstuff.c                              | 136 +++++++++++++
>>  gdb/common/netstuff.h                              |  52 +++++
>>  gdb/doc/gdb.texinfo                                |  48 ++++-
>>  gdb/gdbserver/Makefile.in                          |   2 +
>>  gdb/gdbserver/gdbreplay.c                          | 181 +++++++++++++----
>>  gdb/gdbserver/remote-utils.c                       | 119 +++++++----
>>  gdb/ser-tcp.c                                      | 217 ++++++++++-----------
>>  gdb/testsuite/README                               |   7 +
>>  gdb/testsuite/boards/gdbserver-base.exp            |   5 +
>>  gdb/testsuite/boards/native-extended-gdbserver.exp |   7 +-
>>  gdb/testsuite/boards/native-gdbserver.exp          |   7 +-
>>  .../gdb.server/run-without-local-binary.exp        |   2 +-
>>  14 files changed, 602 insertions(+), 187 deletions(-)
>>  create mode 100644 gdb/common/netstuff.c
>>  create mode 100644 gdb/common/netstuff.h
>> 
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index df6ebab851..06ce12a4ee 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -962,6 +962,7 @@ COMMON_SFILES = \
>>  	common/job-control.c \
>>  	common/gdb_tilde_expand.c \
>>  	common/gdb_vecs.c \
>> +	common/netstuff.c \
>>  	common/new-op.c \
>>  	common/pathstuff.c \
>>  	common/print-utils.c \
>> @@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
>>  	common/gdb_vecs.h \
>>  	common/gdb_wait.h \
>>  	common/common-inferior.h \
>> +	common/netstuff.h \
>>  	common/host-defs.h \
>>  	common/pathstuff.h \
>>  	common/print-utils.h \
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index cef558039e..1f95ced912 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -3,6 +3,10 @@
>>  
>>  *** Changes since GDB 8.1
>>  
>> +* GDB and GDBserver now support IPv6 connections.  IPv6 hostnames
>> +  can be passed using the '[ADDRESS]:PORT' notation, or the regular
>> +  'ADDRESS:PORT' method.
>
> Saying "IPv6 hostnames" and then talking about "ADDRESS:PORT" is
> confusing, I think.  If we're talking about host names then saying
> HOST:PORT would more accurate.  But I think that what you really
> mean is to say "IPv6 _addresses_ can be passed".

That's more correct, indeed.  I've changed the text.

> Does connecting with "localhost6:port" default to IPv6, BTW?
> At least fedora includes "localhost6" in /etc/hosts.

Using "localhost6:port" works, but it doesn't default to IPv6.  Here's
what I see on the gdbserver side:

  $ ./gdb/gdbserver/gdbserver --once localhost6:1234 a.out
  Process /path/to/a.out created; pid = 7742
  Listening on port 1234
  Remote debugging from host ::ffff:127.0.0.1, port 39196

This means that the connection came using IPv4; it works because IPv6
sockets also listen for IPv4 connection on Linux (one can change this
behaviour by setting the "IPV6_V6ONLY" socket option).

This happens because I've made a decision to default to AF_INET (instead
of AF_UNSPEC) when no prefix has been given.  This basically means that,
at least for now, we assume that an unknown (i.e., not prefixed)
address/hostname is IPv4.  I've made this decision thinking about the
convenience of the user: when AF_UNSPEC is used (and the user hasn't
specified any prefix), getaddrinfo will return a linked list of possible
addresses that we should try to connect to, which usually means an IPv6
and an IPv4 address, in that order.  Usually this is fine, because (as I
said) IPv6 sockets can also listen for IPv4 connections.  However, if
you start gdbserver with an explicit IPv4 address:

  $ ./gdb/gdbserver/gdbserver --once 127.0.0.1:1234 a.out

and try to connect GDB to it using an "ambiguous" hostname:

  $ ./gdb/gdb -ex 'target remote localhost:1234' a.out

you will notice that GDB will take a somewhat long time trying to
connect (to the IPv6 address, because of AF_UNSPEC), and then it will
error out saying that the connection timed out:

  tcp:localhost:1234: Connection timed out.

This is because of the auto-retry mechanism implemented for TCP
connections on GDB; it keeps retrying to connect to the IPv6 until it
decides it's not going to work.  Only after this timeout is that GDB
will try to connect to the IPv4 address, and succeed.

So, the way I see it, we have a few options to deal with this scenario:

1) Assume that the unprefixed address/hostname is AF_INET (i.e., keep
the patch as-is).

2) Don't assume anything about the unprefixed address/hostname (i.e.,
AF_UNSPEC), and don't change the auto-retry system.  This is not very
nice because of what I explained above.

3) Don't assume anything about the unprefixed address/hostname (i.e.,
AF_UNSPEC), but *DO* change the auto-retry system to retry less times
(currently it's set to 15 retries, which seems too much to me).  Maybe 5
times is enough?  This will still have an impact on the user, but she
will have to wait less time, at least.

Either (1) or (3) are fine by me.  If we go with (1), we'll eventually
need to change the default to IPv6 (or to AF_UNSPEC), but that's only
when IPv6 is more adopted.

Side note: while I was writing this, I noticed a problem in the code and
fixed it.  Basically, on net_open, should have been declared inside the
for loop, and not outside.  This was making GDB not attempt connecting
the IPv4 address after the IPv6 failed.  Now it's fixed.

>> +/* See common/netstuff.h.  */
>> +
>> +void
>> +parse_hostname_without_prefix (const char *hostname, std::string &host_str,
>> +			       std::string &port_str, struct addrinfo *hint)
>> +{
>> +  std::string strname (hostname);
>
> I suspect the local parsing can be written using
> gdb::string_view to avoid copying?

Hm, I'll investigate it.  So many shiny new features available to us
now, it's hard to keep track!

>> +
>> +/* See common/netstuff.h.  */
>> +
>> +void
>> +parse_hostname (const char *hostname, std::string &host_str,
>> +		std::string &port_str, struct addrinfo *hint)
>> +{
>> +  /* Struct to hold the association between valid prefixes, their
>> +     family and socktype.  */
>> +  struct host_prefix
>> +    {
>> +      /* The prefix.  */
>> +      const char *prefix;
>> +
>> +      /* The 'ai_family'.  */
>> +      int family;
>> +
>> +      /* The 'ai_socktype'.  */
>> +      int socktype;
>> +    };
>> +  static const struct host_prefix prefixes[] =
>> +    {
>> +      { "udp:",  AF_UNSPEC, SOCK_DGRAM },
>> +      { "tcp:",  AF_UNSPEC, SOCK_STREAM },
>> +      { "udp4:", AF_INET,   SOCK_DGRAM },
>> +      { "tcp4:", AF_INET,   SOCK_STREAM },
>> +      { "udp6:", AF_INET6,  SOCK_DGRAM },
>> +      { "tcp6:", AF_INET6,  SOCK_STREAM },
>> +      { NULL, 0, 0 },
>> +    };
>> +
>> +  for (const struct host_prefix *prefix = prefixes;
>> +       prefix->prefix != NULL;
>> +       ++prefix)
>
> I think you could drop the last/null entry and use range-for ?

I remember trying to use a rage-for but having issues.  I think I was
using a different setup for this struct...  Anyway, I implemented the
range-for idea now.

>> +    if (startswith (hostname, prefix->prefix))
>> +      {
>> +	hostname += strlen (prefix->prefix);
>> +	hint->ai_family = prefix->family;
>> +	hint->ai_socktype = prefix->socktype;
>> +	hint->ai_protocol
>> +	  = hint->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
>> +	break;
>> +      }
>> +
>> +  parse_hostname_without_prefix (hostname, host_str, port_str, hint);
>> +}
>
>
>> +/* Parse HOSTNAME (which is a string in the of "ADDR:PORT") and fill
>
> Missing "form" in "in the of".

Fixed.

>> +   in HOST_STR, PORT_STR and HINT accordingly.  */
>> +extern void parse_hostname_without_prefix (const char *hostname,
>> +					   std::string &host_str,
>> +					   std::string &port_str,
>> +					   struct addrinfo *hint);
>> +
>> +/* Parse HOSTNAME (which is a string in the form of
>> +   "[tcp[6]:|udp[6]:]ADDR:PORT") and fill in HOST_STR, PORT_STR and
>> +   HINT accordingly.  */
>> +extern void parse_hostname (const char *hostname, std::string &host_str,
>> +			    std::string &port_str, struct addrinfo *hint);
>
> Really not a big deal, but instead of output parameters, I'd
> consider returning all outputs via return.  Something like:
>
> struct parsed_hostname
> {
>   std::string host_str;
>   std::string port_str;
>   struct addrinfo addrinfo;
> };
> extern parsed_hostname parse_hostname (const char *hostname,
>                                        const struct addrinfo &hint);

Sure, I can do it.

>>  For example, to connect to port 2828 on a terminal server named
>>  @code{manyfarms}:
>> @@ -20514,6 +20525,24 @@ For example, to connect to port 2828 on a terminal server named
>>  target remote manyfarms:2828
>>  @end smallexample
>>  
>> +To connect to port 2828 on a terminal server whose address is
>> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
>> +
>> +@smallexample
>> +target remote [2001::f8ff::67cf]:2828
>> +@end smallexample
>> +
>> +Or explicitly specify the @acronym{IPv6} protocol:
>> +
>> +@smallexample
>> +target remote tcp6:2001::f8ff::67cf:2828
>> +@end smallexample
>> +
>> +This last example may be confusing to the reader, because there is no
>> +visible separation between the hostname and the port number.
>
> Is that really true?  It seems there's visible separation to me -- the
> address/hosthoname part uses double colon, while the port name is
> separated by a single colon?

Ah, I think that's not a good address to use as an example.  I'll use a
"regular" address without double colons.

>> +Therefore, we recommend the user to provide @acronym{IPv6} addresses
>> +using square brackets for clarity.
>> +
>
>>  #ifndef HAVE_SOCKLEN_T
>> @@ -175,56 +176,159 @@ remote_close (void)
>>  static void
>>  remote_open (char *name)
>>  {
>> -  if (!strchr (name, ':'))
>> +  char *last_colon = strrchr (name, ':');
>> +
>> +  if (last_colon == NULL)
>>      {
>>        fprintf (stderr, "%s: Must specify tcp connection as host:addr\n", name);
>>        fflush (stderr);
>>        exit (1);
>>      }
>> -  else
>> -    {
>> +
>>  #ifdef USE_WIN32API
>> -      static int winsock_initialized;
>> +  static int winsock_initialized;
>>  #endif
>> -      char *port_str;
>> -      int port;
>> -      struct sockaddr_in sockaddr;
>> -      socklen_t tmp;
>> -      int tmp_desc;
>> +  char *port_str;
>> +  int tmp;
>> +  int tmp_desc;
>> +  struct addrinfo hint;
>> +  struct addrinfo *ainfo;
>> +  char *orig_name = strdup (name);
>
> Do we need a deep copy?  And if we do, how about
> using std::string to avoid having to call free further
> down?

This is gdbserver/gdbreplay.c, where apparently we don't have access to
a lot of our regular facilities on GDB.  For example, I was trying to
use std::string, its methods, and other stuff here (even i18n
functions), but the code won't compile, and as far as I have researched
this is intentional, because gdbreplay needs to be a very small and
simple program.  Or at least that's what I understood from our
archives/documentation.  I did not feel confident reworking gdbreplay to
make it "modern", so I decided to implement things "the old way".

>> +
>> +  struct prefix
>> +  {
>> +    /* The prefix to be parsed.  */
>> +    const char *str;
>> +
>> +    /* The address family.  */
>> +    int ai_family;
>> +
>> +    /* The socktype.  */
>> +    int ai_socktype;
>> +  };
>> +  static const struct prefix prefixes[]
>> +    = { { "udp:",  AF_UNSPEC, SOCK_DGRAM },
>> +	{ "tcp:",  AF_UNSPEC, SOCK_STREAM },
>> +	{ "udp4:", AF_INET,   SOCK_DGRAM },
>> +	{ "tcp4:", AF_INET,   SOCK_STREAM },
>> +	{ "udp6:", AF_INET6,  SOCK_DGRAM },
>> +	{ "tcp6:", AF_INET6,  SOCK_STREAM },
>> +	{ NULL, 0, 0 } };
>
> That seems like unusual formatting.  In common/netstuff.c
> you broke the starting and ending '{' }' differently.

I'll improve the formatting here.

> I wonder though, shouldn't this be using the new
> netstuff.c shared routines?  It looks like duplicated code?

The reason here is the same as the one I wrote above: doing '#include
"netstuff.h"' brings in a lot of other stuff which break the build of
gdbreplay.c.  That's why I reimplemented the code (and yeah, it is code
duplication, and I wasn't happy about it, but as I said it feels that
these limitations are intentional on gdbreplay.c).

>> +
>> +  memset (&hint, 0, sizeof (hint));
>> +  /* Assume no prefix will be passed, therefore we should use
>> +     AF_UNSPEC.  */
>> +  hint.ai_family = AF_UNSPEC;
>> +  hint.ai_socktype = SOCK_STREAM;
>> +  hint.ai_protocol = IPPROTO_TCP;
>> +
>> +  for (const struct prefix *p = prefixes; p->str != NULL; ++p)
>
> Same comment about range-for.

Fixed.

>> +
>> +  if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
>> +    perror_with_name ("Can't bind address");
>> +
>> +  if (p->ai_socktype == SOCK_DGRAM)
>> +    remote_desc = tmp_desc;
>> +  else
>> +    {
>> +      struct sockaddr_storage sockaddr;
>> +      socklen_t sockaddrsize = sizeof (sockaddr);
>> +      char orig_host[64], orig_port[16];
>
> I guess these magic sizes are garanteed to be enough, since
> you specify NI_NUMERICHOST | NI_NUMERICSERV.  Correct?
> A comment or giving those constants names or comments
> would be good.  Something like:
>
> /* Like NI_MAXHOST/NI_MAXSERV, but enough for numeric forms.  */
> #define GDB_NI_MAX_ADDR 64
> #define GDB_NI_MAX_PORT 16

Good idea, I'll do it.

>>  #if __QNX__
>> @@ -156,19 +159,18 @@ enable_async_notification (int fd)
>>  static int
>>  handle_accept_event (int err, gdb_client_data client_data)
>>  {
>> -  struct sockaddr_in sockaddr;
>> -  socklen_t tmp;
>> +  struct sockaddr_storage sockaddr;
>> +  socklen_t len = sizeof (sockaddr);
>>  
>>    if (debug_threads)
>>      debug_printf ("handling possible accept event\n");
>>  
>> -  tmp = sizeof (sockaddr);
>> -  remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
>> +  remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &len);
>>    if (remote_desc == -1)
>>      perror_with_name ("Accept failed");
>>  
>>    /* Enable TCP keep alive process. */
>> -  tmp = 1;
>> +  socklen_t tmp = 1;
>>    setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
>>  	      (char *) &tmp, sizeof (tmp));
>>  
>> @@ -197,8 +199,19 @@ handle_accept_event (int err, gdb_client_data client_data)
>>    delete_file_handler (listen_desc);
>>  
>>    /* Convert IP address to string.  */
>> -  fprintf (stderr, "Remote debugging from host %s\n",
>> -	   inet_ntoa (sockaddr.sin_addr));
>> +  char orig_host[64], orig_port[16];
>
> Same comment as for gdbreplay.

Consider it done.

>> +
>> +  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);
>
> While at it, couple you please add the missing _() for i18n.

Done.

> BTW, is that line too long?  Can't tell from email client.

78 chars with _() added.  I've decided to break it anyway.

>>  
>> @@ -354,18 +399,24 @@ remote_open (const char *name)
>>  #endif /* USE_WIN32API */
>>    else
>>      {
>> -      int port;
>> -      socklen_t len;
>> -      struct sockaddr_in sockaddr;
>> -
>> -      len = sizeof (sockaddr);
>> -      if (getsockname (listen_desc,
>> -		       (struct sockaddr *) &sockaddr, &len) < 0
>> -	  || len < sizeof (sockaddr))
>> +      char listen_port[16];
>> +      struct sockaddr_storage sockaddr;
>> +      socklen_t len = sizeof (sockaddr);
>> +
>> +      if (getsockname (listen_desc, (struct sockaddr *) &sockaddr, &len) < 0)
>>  	perror_with_name ("Can't determine port");
>> -      port = ntohs (sockaddr.sin_port);
>>  
>> -      fprintf (stderr, "Listening on port %d\n", port);
>> +      int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
>> +			   NULL, 0,
>> +			   listen_port, sizeof (listen_port),
>> +			   NI_NUMERICSERV);
>> +
>> +      if (r != 0)
>> +	fprintf (stderr, _("Can't obtain port where we are listening: %s"),
>> +		 gai_strerror (r));
>> +      else
>> +	fprintf (stderr, "Listening on port %s\n", listen_port);
>
> Preexisting, but while at it, adding the _() wouldn't hurt.

Done.

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/