Re: [PATCH v1 07/12] NetworkPkg: Fix conditionally uninitialized variables

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi Michael,

Comment below.

Mike

> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Erich McMillan <emcmillan@microsoft.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda
> <maciej.rabeda@linux.intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>; Siyuan Fu <siyuan.fu@intel.com>
> Subject: [PATCH v1 07/12] NetworkPkg: Fix conditionally uninitialized variables
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Fixes CodeQL alerts for CWE-457:
> https://cwe.mitre.org/data/definitions/457.html
> 
> Cc: Erich McMillan <emcmillan@microsoft.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Kubacki <mikuback@linux.microsoft.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Co-authored-by: Erich McMillan <emcmillan@microsoft.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c | 2 +-
>  NetworkPkg/TcpDxe/TcpInput.c               | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> index 6a5d78629bb3..71c98abc820e 100644
> --- a/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -753,7 +753,7 @@ HttpUrlGetPort (
> 
>    Status =  AsciiStrDecimalToUintnS (Url + Parser->FieldData[HTTP_URI_FIELD_PORT].Offset, (CHAR8 **)NULL, &Data);
> 
> -  if (Data > HTTP_URI_PORT_MAX_NUM) {
> +  if (!EFI_ERROR (Status) && (Data > HTTP_URI_PORT_MAX_NUM)) {

I do not think this logic change is correct.  If the string can not be
converted to  a value, then Status will be an error.  If that happens,
then the value of Data is undefined.  An error should be returned if
Status is an error or Data is out of range.

if (EFI_ERROR (Status) || (Data > HTTP_URI_PORT_MAX_NUM)) {

>      Status = EFI_INVALID_PARAMETER;
>      goto ON_EXIT;
>    }
> diff --git a/NetworkPkg/TcpDxe/TcpInput.c b/NetworkPkg/TcpDxe/TcpInput.c
> index fb1aa827f8ba..7b329be64dfe 100644
> --- a/NetworkPkg/TcpDxe/TcpInput.c
> +++ b/NetworkPkg/TcpDxe/TcpInput.c
> @@ -1570,6 +1570,9 @@ TcpIcmpInput (
>    BOOLEAN     IcmpErrIsHard;
>    BOOLEAN     IcmpErrNotify;
> 
> +  IcmpErrIsHard = FALSE;
> +  IcmpErrNotify = FALSE;
> +
>    if (Nbuf->TotalSize < sizeof (TCP_HEAD)) {
>      goto CLEAN_EXIT;
>    }
> --
> 2.28.0.windows.1