Re: [Patch] NetworkPkg/HttpDxe: Stripped square brackets in IPv6 expressed HostName.

[Laszlo Ersek <lersek@redhat.com>]

On 08/01/18 03:54, Jiaxin Wu wrote:
> In URI, the colon (:) is used to terminate the HostName path before
> a port number. However, if HostName is expressed as IPv6 format, colon
> characters in IPv6 addresses will conflict with the colon before port
> number. To alleviate this conflict in URI, the IPv6 expressed HostName
> are enclosed in square brackets ([]). To record the real IPv6 HostName,
> square brackets should be stripped.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  NetworkPkg/HttpDxe/HttpImpl.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
> index 17deceb395..e05ee9344b 100644
> --- a/NetworkPkg/HttpDxe/HttpImpl.c
> +++ b/NetworkPkg/HttpDxe/HttpImpl.c
> @@ -403,14 +403,25 @@ EfiHttpRequest (
>      Status = HttpParseUrl (Url, (UINT32) AsciiStrLen (Url), FALSE, &UrlParser);
>      if (EFI_ERROR (Status)) {
>        goto Error1;
>      }
>  
> -    HostName   = NULL;
> -    Status     = HttpUrlGetHostName (Url, UrlParser, &HostName);
> +    Status = HttpUrlGetHostName (Url, UrlParser, &HostName);
>      if (EFI_ERROR (Status)) {
> -     goto Error1;
> +      goto Error1;
> +    }
> +
> +    if (HttpInstance->LocalAddressIsIPv6 && AsciiStrSize (HostName) > 2 &&
> +        HostName[0] == '[' && *(HostName + (AsciiStrSize (HostName) - 2)) == ']') {
> +      //
> +      // HostName format is expressed as IPv6, so, remove '[' and ']'.
> +      //
> +      HostNameSize = AsciiStrSize (HostName) - 2;
> +
> +      CopyMem (HostName, HostName + 1, HostNameSize - 1);
> +
> +      *(HostName + HostNameSize - 1) = '\0';
>      }
>  
>      Status = HttpUrlGetPort (Url, UrlParser, &RemotePort);
>      if (EFI_ERROR (Status)) {
>        if (HttpInstance->UseHttps) {
> 

There are a number of expressions of the form

  *(HostName + Offset)

which could be rewritten more idiomatically as

  HostName[Offset]

In addition, I think the code could be optimized by calculating
AsciiStrSize() only once:

  if (HttpInstance->LocalAddressIsIPv6) {
    HostNameSize = AsciiStrSize (HostName);

    if (HostNameSize > 2 &&
        HostName[0] == '[' &&
        HostName[HostNameSize - 2] == ']') {
      //
      // HostName format is expressed as IPv6, so, remove '[' and ']'.
      //
      HostNameSize -= 2;
      CopyMem (HostName, HostName + 1, HostNameSize - 1);
      HostName[HostNameSize - 1] = '\0';
    }
  }

Under my proposal, if the inner condition fails, then "HostNameSize"
will be set as a "side effect", but I don't think that's a problem.


Anyway, the patch seems technically correct; if you don't want to submit
a v2, I'm fine with this variant too:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I have just one request for the subject line, before you push the patch:
please replace "stripped" with "strip".

Thanks!
Laszlo