NetworkPkg: Addressed static code analyzer issues

[edk2-devel] [PATCH] NetworkPkg: Addressed static code analyzer issues

Posted by INDIA\sivaramann 2 years, 10 months ago

Issue on the PxeBcDhcp4CallBack() functions of UEFIPXEBC Driver.
In this function allowed events are Dhcp4RcvdOffer, Dhcp4SelectOffer,
Dhcp4SendDiscover, Dhcp4RcvdAck. If any other event comes as input
it will exit in beginning itself.

Later below switch case handling the default case which is not reachable.
I assume this code is a not reachable code and can be removed

Signed-off-by: Sivaraman <sivaramann@ami.com>
---
 NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
index fb63cf61a9..c0d8211ea0 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
@@ -1331,8 +1331,6 @@ PxeBcDhcp4CallBack (
     }
     break;
 
-  default:
-    break;
   }
 
   return Status;
-- 
2.28.0.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76788): https://edk2.groups.io/g/devel/message/76788
Mute This Topic: https://groups.io/mt/83671978/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] NetworkPkg: Addressed static code analyzer issues

Posted by Laszlo Ersek 2 years, 10 months ago

adding NetworkPkg maintainers, comments below

On 06/18/21 05:30, INDIA\sivaramann wrote:
> Issue on the PxeBcDhcp4CallBack() functions of UEFIPXEBC Driver.
> In this function allowed events are Dhcp4RcvdOffer, Dhcp4SelectOffer,
> Dhcp4SendDiscover, Dhcp4RcvdAck. If any other event comes as input
> it will exit in beginning itself.

Yes.

> 
> Later below switch case handling the default case which is not reachable.
> I assume this code is a not reachable code and can be removed

(1) The edk2 coding style recommends adding "default" cases to switch
statements, as far as I recall. I'd keep the default, but add

  ASSERT (FALSE);

there.

(2) There is a more confusing style issue with the same switch
statement. Namely, it has a case label for "Dhcp4SendRequest". Control
will never jump to that label, due to the "if" at the top of the
function that you highlight.

Importantly, the *code* starting at the "Dhcp4SendRequest" case label
must not be removed, as the "Dhcp4SendDiscover" logic *falls through* to
it. However, the "Dhcp4SendRequest" case label itself should be removed,
and the comment just above it should be updated.

This dead label seems to originate from historical commit a3bcde70e6dc
("Add NetworkPkg (P.UDK2010.UP3.Network.P1)", 2010-11-01).

(3) The subject line is nearly useless, please name at least
"NetworkPkg/UefiPxeBcDxe".

Thanks
Laszlo

> 
> Signed-off-by: Sivaraman <sivaramann@ami.com>
> ---
>  NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> index fb63cf61a9..c0d8211ea0 100644
> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> @@ -1331,8 +1331,6 @@ PxeBcDhcp4CallBack (
>      }
>      break;
>  
> -  default:
> -    break;
>    }
>  
>    return Status;
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76924): https://edk2.groups.io/g/devel/message/76924
Mute This Topic: https://groups.io/mt/83671978/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg: Addressed static code analyzer issues

Posted by Sivaraman Nainar 2 years, 10 months ago

Hi Lazlo:

  case Dhcp4SendRequest:
    if (Packet->Length > PXEBC_DHCP4_PACKET_MAX_SIZE) {
      //
      // If the to be sent packet exceeds the maximum length, abort the DHCP process.
      //
      Status = EFI_ABORTED;
      break;
    }
The same packet Length check already taken care on case Dhcp4SendDiscover: also.

So I prefer to remove the above set of lines.

Wil update the subject when sending V2 changes.

-Siva
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Wednesday, June 23, 2021 3:45 PM
To: devel@edk2.groups.io; emergingsiva@gmail.com
Cc: Sivaraman Nainar <sivaramann@ami.com>; Maciej Rabeda <maciej.rabeda@linux.intel.com>; Jiaxin Wu <jiaxin.wu@intel.com>; Siyuan Fu <siyuan.fu@intel.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] NetworkPkg: Addressed static code analyzer issues


**CAUTION: The e-mail below is from an external source. Please exercise caution before opening attachments, clicking links, or following guidance.**

adding NetworkPkg maintainers, comments below

On 06/18/21 05:30, INDIA\sivaramann wrote:
> Issue on the PxeBcDhcp4CallBack() functions of UEFIPXEBC Driver.
> In this function allowed events are Dhcp4RcvdOffer, Dhcp4SelectOffer,
> Dhcp4SendDiscover, Dhcp4RcvdAck. If any other event comes as input it
> will exit in beginning itself.

Yes.

>
> Later below switch case handling the default case which is not reachable.
> I assume this code is a not reachable code and can be removed

(1) The edk2 coding style recommends adding "default" cases to switch statements, as far as I recall. I'd keep the default, but add

  ASSERT (FALSE);

there.

(2) There is a more confusing style issue with the same switch statement. Namely, it has a case label for "Dhcp4SendRequest". Control will never jump to that label, due to the "if" at the top of the function that you highlight.

Importantly, the *code* starting at the "Dhcp4SendRequest" case label must not be removed, as the "Dhcp4SendDiscover" logic *falls through* to it. However, the "Dhcp4SendRequest" case label itself should be removed, and the comment just above it should be updated.

This dead label seems to originate from historical commit a3bcde70e6dc ("Add NetworkPkg (P.UDK2010.UP3.Network.P1)", 2010-11-01).

(3) The subject line is nearly useless, please name at least "NetworkPkg/UefiPxeBcDxe".

Thanks
Laszlo


>
> Signed-off-by: Sivaraman <sivaramann@ami.com>
> ---
>  NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> index fb63cf61a9..c0d8211ea0 100644
> --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c
> @@ -1331,8 +1331,6 @@ PxeBcDhcp4CallBack (
>      }
>      break;
>
> -  default:
> -    break;
>    }
>
>    return Status;
>

-The information contained in this message may be confidential and proprietary to American Megatrends (AMI). This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76966): https://edk2.groups.io/g/devel/message/76966
Mute This Topic: https://groups.io/mt/83735967/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-