Re: [PATCH v4 02/19] net: read bracketed ipv6 addrs and port numbers

[Daniel Kiper]

On Mon, Jan 09, 2023 at 06:30:28PM -0500, Robbie Harwood wrote:
> From: Aaron Miller <aaronmiller@fb.com>
>
> Allow specifying port numbers for http and tftp paths, and allow ipv6
> addresses to be recognized with brackets around them, which is required
> to specify a port number.
>
> Signed-off-by: Aaron Miller <aaronmiller@fb.com>
> Co-authored-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  grub-core/net/http.c | 25 ++++++++++---
>  grub-core/net/net.c  | 87 +++++++++++++++++++++++++++++++++++++++++---
>  grub-core/net/tftp.c |  7 +++-
>  include/grub/net.h   |  1 +

Please document this feature in the docs/grub.texi as it was in the
patch you reverted earlier.

>  4 files changed, 109 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index d67cad4829..e76c0e4b62 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -295,7 +295,9 @@ http_receive (grub_net_tcp_socket_t sock __attribute__ 
> ((unused)),
>         nb2 = grub_netbuff_alloc (data->chunk_rem);
>         if (!nb2)
>           return grub_errno;
> -       grub_netbuff_put (nb2, data->chunk_rem);
> +       err = grub_netbuff_put (nb2, data->chunk_rem);
> +       if (err)
> +         return grub_errno;

Hmmm... This change does not seem to belong to this patch...

>         grub_memcpy (nb2->data, nb->data, data->chunk_rem);
>         if (file->device->net->packs.count >= 20)
>           {
> @@ -318,12 +320,14 @@ http_establish (struct grub_file *file, grub_off_t 
> offset, int initial)
>    int i;
>    struct grub_net_buff *nb;
>    grub_err_t err;
> +  char* server = file->device->net->server;

s/* server/ *server/

> +  int port = file->device->net->port;
>
>    nb = grub_netbuff_alloc (GRUB_NET_TCP_RESERVE_SIZE
>                          + sizeof ("GET ") - 1
>                          + grub_strlen (data->filename)
>                          + sizeof (" HTTP/1.1\r\nHost: ") - 1
> -                        + grub_strlen (file->device->net->server)
> +                        + grub_strlen (server) + sizeof (":XXXXXXXXXX")
>                          + sizeof ("\r\nUser-Agent: " PACKAGE_STRING
>                                    "\r\n") - 1
>                          + sizeof ("Range: bytes=XXXXXXXXXXXXXXXXXXXX"
> @@ -362,7 +366,7 @@ http_establish (struct grub_file *file, grub_off_t 
> offset, int initial)
>              sizeof (" HTTP/1.1\r\nHost: ") - 1);
>
>    ptr = nb->tail;
> -  err = grub_netbuff_put (nb, grub_strlen (file->device->net->server));
> +  err = grub_netbuff_put (nb, grub_strlen (server));
>    if (err)
>      {
>        grub_netbuff_free (nb);
> @@ -371,6 +375,15 @@ http_establish (struct grub_file *file, grub_off_t 
> offset, int initial)
>    grub_memcpy (ptr, file->device->net->server,
>              grub_strlen (file->device->net->server));
>
> +  if (port)
> +    {
> +      ptr = nb->tail;
> +      grub_snprintf ((char *) ptr,
> +       sizeof (":XXXXXXXXXX"),
> +       ":%d",
> +       port);

Please put this in one line.

> +    }
> +
>    ptr = nb->tail;
>    err = grub_netbuff_put (nb,
>                         sizeof ("\r\nUser-Agent: " PACKAGE_STRING "\r\n")
> @@ -396,8 +409,10 @@ http_establish (struct grub_file *file, grub_off_t 
> offset, int initial)
>    grub_netbuff_put (nb, 2);
>    grub_memcpy (ptr, "\r\n", 2);
>
> -  data->sock = grub_net_tcp_open (file->device->net->server,
> -                               HTTP_PORT, http_receive,
> +  grub_dprintf ("http", "opening path %s on host %s TCP port %d\n",
> +             data->filename, server, port ? port : HTTP_PORT);
> +  data->sock = grub_net_tcp_open (server,
> +                               port ? port : HTTP_PORT, http_receive,
>                                 http_err, NULL,
>                                 file);
>    if (!data->sock)
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 7046dc5789..c7f4fb6a6f 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -443,6 +443,13 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const 
> char **rest)
>    grub_uint16_t newip[8];
>    const char *ptr = val;
>    int word, quaddot = -1;
> +  int bracketed = 0;

bool?

> +
> +  if (ptr[0] == '[')
> +    {
> +      bracketed = 1;
> +      ptr++;
> +    }
>
>    if (ptr[0] == ':' && ptr[1] != ':')
>      return 0;
> @@ -481,6 +488,8 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const char 
> **rest)
>        grub_memset (&newip[quaddot], 0, (7 - word) * sizeof (newip[0]));
>      }
>    grub_memcpy (ip, newip, 16);
> +  if (bracketed && *ptr == ']')
> +    ptr++;
>    if (rest)
>      *rest = ptr;
>    return 1;
> @@ -1319,8 +1328,10 @@ grub_net_open_real (const char *name)
>  {
>    grub_net_app_level_t proto;
>    const char *protname, *server;
> +  char *host;
>    grub_size_t protnamelen;
>    int try;
> +  int port = 0;
>
>    if (grub_strncmp (name, "pxe:", sizeof ("pxe:") - 1) == 0)
>      {
> @@ -1358,6 +1369,72 @@ grub_net_open_real (const char *name)
>        return NULL;
>      }
>
> +  char *port_start;

Please define this at the beginning of the function.

> +  /* ipv6 or port specified? */
> +  if ((port_start = grub_strchr (server, ':')))
> +    {
> +      char *ipv6_begin;
> +      if ((ipv6_begin = grub_strchr (server, '[')))
> +     {
> +       char *ipv6_end = grub_strchr (server, ']');
> +       if (!ipv6_end)
> +         {
> +           grub_error (GRUB_ERR_NET_BAD_ADDRESS,
> +                          N_("mismatched [ in address"));

Please put this in one line.

> +           return NULL;
> +         }
> +       /* port number after bracketed ipv6 addr */
> +       if (ipv6_end[1] == ':')
> +         {
> +           port = grub_strtoul (ipv6_end + 2, NULL, 10);
> +           if(port > 65535)
> +             {
> +               grub_error (GRUB_ERR_NET_BAD_ADDRESS,
> +                              N_("bad port number"));

Ditto.

> +               return NULL;
> +             }

This error check is not correct. Good example how it should be done is
in the patch you reverted earlier.

> +         }
> +       host = grub_strndup (ipv6_begin, (ipv6_end - ipv6_begin) + 1);
> +     }
> +      else
> +     {
> +       if (grub_strchr (port_start + 1, ':'))
> +         {
> +           int iplen = grub_strlen (server);
> +           /* bracket bare ipv6 addrs */
> +           host = grub_malloc (iplen + 3);
> +           if (!host)
> +             {
> +               return NULL;
> +             }
> +           host[0] = '[';
> +           grub_memcpy (host + 1, server, iplen);
> +           host[iplen + 1] = ']';
> +           host[iplen + 2] = '\0';
> +         }
> +       else
> +         {
> +           /* hostname:port or ipv4:port */
> +           port = grub_strtol (port_start + 1, NULL, 10);
> +           if (port > 65535)
> +             {
> +               grub_error (GRUB_ERR_NET_BAD_ADDRESS,
> +                              N_("bad port number"));

Please put this in one line.

> +               return NULL;
> +             }

Again, this error check is not correct...

> +           host = grub_strndup (server, port_start - server);
> +         }
> +     }
> +    }
> +  else
> +    {
> +      host = grub_strdup (server);
> +    }
> +  if (!host)
> +    {
> +      return NULL;
> +    }

Could you drop redundant curly braces here and a bit above?

> +
>    for (try = 0; try < 2; try++)
>      {
>        FOR_NET_APP_LEVEL (proto)
> @@ -1367,14 +1444,13 @@ grub_net_open_real (const char *name)
>         {
>           grub_net_t ret = grub_zalloc (sizeof (*ret));
>           if (!ret)
> -           return NULL;
> -         ret->protocol = proto;
> -         ret->server = grub_strdup (server);
> -         if (!ret->server)
>             {
> -             grub_free (ret);
> +             grub_free (host);
>               return NULL;
>             }
> +         ret->protocol = proto;
> +         ret->port = port;
> +         ret->server = host;
>           ret->fs = &grub_net_fs;
>           return ret;
>         }
> @@ -1449,6 +1525,7 @@ grub_net_open_real (const char *name)
>    grub_error (GRUB_ERR_UNKNOWN_DEVICE, N_("disk `%s' not found"),
>             name);
>
> +  grub_free (host);
>    return NULL;
>  }
>
> diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
> index 7dbd3056d6..713af2e95b 100644
> --- a/grub-core/net/tftp.c
> +++ b/grub-core/net/tftp.c
> @@ -295,6 +295,7 @@ tftp_open (struct grub_file *file, const char *filename)
>    grub_err_t err;
>    grub_uint8_t *nbd;
>    grub_net_network_level_address_t addr;
> +  int port = file->device->net->port;
>
>    data = grub_zalloc (sizeof (*data));
>    if (!data)
> @@ -361,12 +362,16 @@ tftp_open (struct grub_file *file, const char *filename)
>    err = grub_net_resolve_address (file->device->net->server, &addr);
>    if (err)
>      {
> +      grub_dprintf ("tftp", "Address resolution failed: %d\n", err);
> +      grub_dprintf ("tftp", "file_size is %llu, block_size is %llu\n",
> +                 (unsigned long long)data->file_size,
> +                 (unsigned long long)data->block_size);

Why do not use proper PRIuGRUB_* format specifiers instead of casts here?

>        grub_free (data);
>        return err;
>      }
>
>    data->sock = grub_net_udp_open (addr,
> -                               TFTP_SERVER_PORT, tftp_receive,
> +                               port ? port : TFTP_SERVER_PORT, tftp_receive,
>                                 file);
>    if (!data->sock)
>      {
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 79cba357ae..67661e51e5 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -271,6 +271,7 @@ typedef struct grub_net
>  {
>    char *server;
>    char *name;
> +  int port;

grub_uint16_t port;?

Daniel