grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/8] net: read bracketed ipv6 addrs and port numbers


From: Andrei Borzenkov
Subject: Re: [PATCH 2/8] net: read bracketed ipv6 addrs and port numbers
Date: Wed, 3 Aug 2016 11:17:35 +0300

On Wed, Aug 3, 2016 at 9:34 AM, Michael Chang <address@hidden> wrote:
> From: Aaron Miller <address@hidden>
>
> 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
> ---
>  grub-core/net/http.c | 21 ++++++++++---
>  grub-core/net/net.c  | 86 
> +++++++++++++++++++++++++++++++++++++++++++++++++---
>  grub-core/net/tftp.c |  6 +++-
>  include/grub/net.h   |  1 +
>  4 files changed, 104 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index 5aa4ad3..f182d7b 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -312,12 +312,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;
> +  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")

I just wonder why so many X's; port is 16 bit so 5 digits at most + zero byte.

>                            + sizeof ("\r\nUser-Agent: " PACKAGE_STRING
>                                      "\r\n") - 1
>                            + sizeof ("Range: bytes=XXXXXXXXXXXXXXXXXXXX"
> @@ -356,7 +358,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);
> @@ -365,6 +367,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)

As was discussed previously (probably in another version) zero is
valid port number, although granted it is unusual. This applies also
to all other cases below. It would probably be easier to simply always
use port number.

> +    {
> +      ptr = nb->tail;
> +      grub_snprintf ((char *) ptr,
> +         sizeof (":XXXXXXXXXX"),
> +         ":%d",
> +         port);
> +    }
> +
>    ptr = nb->tail;
>    err = grub_netbuff_put (nb,
>                           sizeof ("\r\nUser-Agent: " PACKAGE_STRING "\r\n")
> @@ -390,8 +401,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,

See above.

>                                   http_err, http_err,
>                                   file);
>    if (!data->sock)
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 10773fc..5cc0d2f 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -437,6 +437,12 @@ 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;
> +
> +  if (ptr[0] == '[') {
> +    bracketed = 1;
> +    ptr++;
> +  }
>

Bracketed IPv6 address is defined for use in URI. So it has to be
parsed where URI is expected. As far as I can tell, the only reason
for this in this patch is to allow ":" as port separator in net device
syntax that can be avoided, see below.

>    if (ptr[0] == ':' && ptr[1] != ':')
>      return 0;
> @@ -475,6 +481,9 @@ 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;
> @@ -1260,8 +1269,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)
>      {
> @@ -1299,6 +1310,72 @@ grub_net_open_real (const char *name)
>        return NULL;
>      }
>
> +  char* port_start;
> +  /* ipv6 or port specified? */
> +  if ((port_start = grub_strchr (server, ':')))

I would probably prefer (proto,address[,port]) syntax to avoid all
ambiguity. The question is, do we want to allow further extending
syntax like user/password, but then we could simply allow empty port
part.

And if bracketed addresses should be supported, just parse them here.

> +  {
> +      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"));
> +             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"));
> +                 return NULL;
> +               }
> +           }
> +         host = grub_strndup (ipv6_begin, (ipv6_end - ipv6_begin) + 1);

This could just as well be ipv6_begin + 1, ipv6_end -1, right?

> +       }
> +      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';
> +           }

Why add brackets just to strip them as the very first step later?

> +         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"));
> +                 return NULL;
> +               }
> +             host = grub_strndup (server, port_start - server);
> +           }
> +       }
> +    }
> +  else
> +    {
> +      host = grub_strdup (server);
> +    }
> +  if (!host)
> +    {
> +      return NULL;
> +    }
> +
>    for (try = 0; try < 2; try++)
>      {
>        FOR_NET_APP_LEVEL (proto)
> @@ -1308,14 +1385,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;

May be we could define default port as part of grub_net_app_protocol.
Then all logic to parse port will be localized here. Everything else
just always has well defined port number.


> +           ret->server = host;
>             ret->fs = &grub_net_fs;
>             return ret;
>           }
> diff --git a/grub-core/net/tftp.c b/grub-core/net/tftp.c
> index 7d90bf6..a0817a0 100644
> --- a/grub-core/net/tftp.c
> +++ b/grub-core/net/tftp.c
> @@ -314,6 +314,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)
> @@ -382,13 +383,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", "file_size is %llu, block_size is %llu\n",
> +                   (unsigned long long)data->file_size,
> +                   (unsigned long long)data->block_size);
>        destroy_pq (data);
>        grub_free (data);
>        return err;
>      }
>
>    data->sock = grub_net_udp_open (addr,
> -                                 TFTP_SERVER_PORT, tftp_receive,
> +                                 port ? port : TFTP_SERVER_PORT, 
> tftp_receive,

See above about zero ports.

>                                   file);
>    if (!data->sock)
>      {
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 2192fa1..ccc169c 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -270,6 +270,7 @@ typedef struct grub_net
>  {
>    char *server;
>    char *name;
> +  int port;
>    grub_net_app_level_t protocol;
>    grub_net_packets_t packs;
>    grub_off_t offset;
> --
> 2.6.6
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]