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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Thanks Laszlo, I have refined the patch to v2 with your "Reviewed-by" tag.



> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, August 1, 2018 5:49 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: Re: [Patch] NetworkPkg/HttpDxe: Stripped square brackets in IPv6
> expressed HostName.
> 
> 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