Re: [PATCH v2] http: Allow use of non-standard TCP/IP ports

[Daniel Kiper]

On Sat, Jan 08, 2022 at 06:22:47PM -0700, Stephen Balousek wrote:
> Allow the use of HTTP servers listening on ports other 80. This is done
> with an extension to the http notation:
>
>   (http[,server[,port]])
>
>  - or -
>
>   (http[,server[:port]])
>
> Signed-off-by: Stephen Balousek <sbalousek@wickedloop.com>
> ---
>
> Hi Daniel,

Hi Stephen,

> Happy New Year!

Happy New Year for you too!

> I apologize for the long delay. I really wanted to test the changes

No worries. I know how it works...

> for an HTTP server over IPv6, but not knowing much about setting up an
> IPv6 network made for a pretty steep learning curve! Anyway, please
> find attached a new version of the patch for allowing the use of
> non-standard HTTP server ports.
>
> I added some comments to the code for parsing the port number from an
> IPv6 address, and added relevant examples to the INFO document.
>
> Also, you mentioned:
>
>     I think it should be "port_number == GRUB_ULONG_MAX" instead of
>     "port_number == 0" here.
>
> I took a closer look at  grub_strtoul() and that function actually
> returns zero (0) when a non-numeric value is encounteded in the
> string. So, functionally, I think these changes are correct.

Yeah, only if there are no digits in the string... Please look below...

> Thank you for your patience and all the help!
> - Steve
>
>
>  docs/grub.texi       | 33 +++++++++++++++++++++++++++++++++
>  grub-core/net/http.c | 39 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index f8b4b3b21..a95d86f95 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3004,6 +3004,39 @@ environment variable @samp{net_default_server} is used.
>  Before using the network drive, you must initialize the network.
>  @xref{Network}, for more information.
>
> +For the @samp{http} network protocol, @code{@var{server}} may specify a
> +port number other than the default value of @samp{80}. The server name
> +and port number are separated by either @samp{,} or @samp{:}.
> +For IPv6 addresses, the server name and port number may only be separated
> +by @samp{,}.
> +
> +@itemize @bullet
> +@item
> +@code{(http,@var{server},@var{port})}
> +
> +@item
> +@code{(http,@var{server}:@var{port})}
> +@end itemize
> +
> +These examples all reference an @samp{http} server at address
> +@samp{192.168.0.100} listening on the non-standard port of @samp{3000}.
> +In these examples, the DNS name @samp{grub.example.com} is resolved
> +to @samp{192.168.0.100}.
> +
> +@example
> +(http,grub.example.com,3000)
> +(http,grub.example.com:3000)
> +(http,192.168.0.100,3000)
> +(http,192.168.0.100:3000)
> +@end example
> +
> +Referencing an @samp{http} server over IPv6 on the non-standard
> +port of @samp{3000} would look like this:
> +
> +@example
> +(http,2001:dead:beef::1,3000)
> +@end example
> +
>  If you boot GRUB from a CD-ROM, @samp{(cd)} is available. @xref{Making
>  a GRUB bootable CD-ROM}, for details.
>
> diff --git a/grub-core/net/http.c b/grub-core/net/http.c
> index b616cf40b..f457cd010 100644
> --- a/grub-core/net/http.c
> +++ b/grub-core/net/http.c
> @@ -312,6 +312,9 @@ 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_name;
> +  char *port_string;
> +  unsigned long port_number;
>
>    nb = grub_netbuff_alloc (GRUB_NET_TCP_RESERVE_SIZE
>                          + sizeof ("GET ") - 1
> @@ -390,10 +393,42 @@ 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,
> +  port_string = grub_strrchr (file->device->net->server, ',');
> +  if (port_string == NULL)
> +    {
> +      /* If ',port' is not found in the http server string, look for ':port' 
> */
> +      port_string = grub_strrchr (file->device->net->server, ':');
> +      /* For IPv6 addresses, the ':port' syntax is not supported and ',port' 
> must be used. */
> +      if (port_string != NULL && grub_strchr (file->device->net->server, 
> ':') != port_string)
> +       port_string = NULL;
> +    }
> +  if (port_string != NULL)
> +    {
> +      port_number = grub_strtoul (port_string + 1, 0, 10);
> +      if (port_number == 0 && grub_errno == GRUB_ERR_BAD_NUMBER)
> +       return grub_error (GRUB_ERR_BAD_NUMBER, N_("non-numeric or invalid 
> port number `%s'"), port_string + 1);

This check is not reliable. If you want to rely on grub_errno you should
reset grub_errno to GRUB_ERR_NONE before grub_strtoul() call. However,
this still does not allow you to detect partially invalid strings. So,
I think you should do this:

port_number = grub_strtoul (port_string + 1, &endptr, 10);
if (*(port_string + 1) == '\0' || endptr != '\0')
  return grub_error (GRUB_ERR_BAD_NUMBER, N_("non-numeric or invalid port 
number `%s'"), port_string + 1);

Otherwise the patch LGTM.

Daniel