From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 7A3AE94134B for ; Thu, 5 Oct 2023 06:31:11 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=RZ5wDMofn83716NhYUbNTuMy7qXYNuTGPI3nV1IT/Aw=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1696487470; v=1; b=YEatJghswpiIA6Mq2fEmefZPN9QiK1zjw1Y3sAZKItTL7/fe3ee9ptM9TO0uIpeJGtd4K5Uo D9KkMt7/f01YDRL9Ft1Z0AQf3Ob7mCbVOw9o8c6IUv1VNtm/3PTiLTlWE9kfhXpvXZRDWbKHhuS Wq0xpkE3h0HBgVzX3zHmrCqU= X-Received: by 127.0.0.2 with SMTP id 3f1MYY7687511xC9ycDz5Bhq; Wed, 04 Oct 2023 23:31:10 -0700 X-Received: from mail-pg1-f179.google.com (mail-pg1-f179.google.com [209.85.215.179]) by mx.groups.io with SMTP id smtpd.web10.9572.1696487469323877957 for ; Wed, 04 Oct 2023 23:31:09 -0700 X-Received: by mail-pg1-f179.google.com with SMTP id 41be03b00d2f7-5809d5fe7f7so408584a12.3 for ; Wed, 04 Oct 2023 23:31:09 -0700 (PDT) X-Gm-Message-State: ZU06rYcSF6XRg36cILwttnDNx7686176AA= X-Google-Smtp-Source: AGHT+IEbnsi1RNTb7IcN/o45PVOCBC09+9+YVavQTmvDtuGYpZriGGQLAW+aTxrDHKHr9p5WDZZ7bTYAfE5Q87ZfJBI= X-Received: by 2002:a17:90a:c293:b0:263:f62b:3601 with SMTP id f19-20020a17090ac29300b00263f62b3601mr3929997pjt.10.1696487468280; Wed, 04 Oct 2023 23:31:08 -0700 (PDT) MIME-Version: 1.0 References: <20231004054818.100353-1-rsingh@ventanamicro.com> <20231004054818.100353-3-rsingh@ventanamicro.com> In-Reply-To: From: "Ranbir Singh" Date: Thu, 5 Oct 2023 12:00:57 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: Fix MISSING_BREAK Coverity issues To: "Kinney, Michael D" Cc: "devel@edk2.groups.io" , "Wu, Hao A" , "Ni, Ray" , Veeresh Sangolli Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,rsingh@ventanamicro.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="000000000000e9c0630606f24479" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=YEatJghs; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --000000000000e9c0630606f24479 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Mike, I earlier assumed that both the fallthrough were intentional and just made it evident by writing the comment. It was hence written actually for the code reader and not the static analyzer. The comment was similar in nature and style to existing No/1-byte/... data comments. *Incidentally, the comment satisfied the static analyzer as well. *It was beyond my expectation and I was pleasantly surprised too. Without digging deep into the particular code and to avoid any functional code change, I just thought that's it. Your comment forced me to come back specifically to the particular code and to analyze it a bit more. My analysis now says that the fallthrough doesn't serve any real functional purpose as there is absolutely no chance of *if* blocks of the fallthrough *case(*s) being executed later and not executed in the original *case:* and hence the fallthrough if it happens still leads to execution of *return NULL; *at the end of the function only. This is better communicated by adding *break;* statement in the original *case:* and in a way, one may agree with the static analyzer that there was a MISSING_BREAK. I should hence follow it up with a v2 patch for this. Ranbir Singh On Thu, Oct 5, 2023 at 12:17=E2=80=AFAM Kinney, Michael D < michael.d.kinney@intel.com> wrote: > I do not prefer special comments for one static analyzer. > > Is there an alternative design/implementation of this code > to make it more readable and not trigger any static analysis > false positives? > > Mike > > > -----Original Message----- > > From: devel@edk2.groups.io On Behalf Of Ranbir > > Singh > > Sent: Tuesday, October 3, 2023 10:48 PM > > To: devel@edk2.groups.io; rsingh@ventanamicro.com > > Cc: Wu, Hao A ; Ni, Ray ; > > Veeresh Sangolli > > Subject: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe: > > Fix MISSING_BREAK Coverity issues > > > > From: Ranbir Singh > > > > The function GetNextHidItem has a switch-case code in which the > > case 1: falls through to case 2: and then case 2: falls through > > to case 3: in the block. > > > > While this may be intentional, it is not evident to any general > > code reader as well as any static analyzer tool. Just adding > > > > // No break; here as this is an intentional fallthrough. > > > > as comment in between makes a reader as well as Coverity happy. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D4222 > > > > Cc: Hao A Wu > > Cc: Ray Ni > > Co-authored-by: Veeresh Sangolli > > Signed-off-by: Ranbir Singh > > Signed-off-by: Ranbir Singh > > --- > > MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c > > b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c > > index acc19acd98e0..bc9a4824208b 100644 > > --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c > > +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c > > @@ -89,6 +89,10 @@ GetNextHidItem ( > > return StartPos; > > > > } > > > > > > > > + // > > > > + // No break; here as this is an intentional fallthrough > > > > + // > > > > + > > > > case 2: > > > > // > > > > // 2-byte data > > > > @@ -99,6 +103,10 @@ GetNextHidItem ( > > return StartPos; > > > > } > > > > > > > > + // > > > > + // No break; here as this is an intentional fallthrough > > > > + // > > > > + > > > > case 3: > > > > // > > > > // 4-byte data, adjust size > > > > -- > > 2.34.1 > > > > > > > > -=3D-=3D-=3D-=3D-=3D-=3D > > Groups.io Links: You receive all messages sent to this group. > > View/Reply Online (#109309): > > https://edk2.groups.io/g/devel/message/109309 > > Mute This Topic: https://groups.io/mt/101750274/1643496 > > Group Owner: devel+owner@edk2.groups.io > > Unsubscribe: https://edk2.groups.io/g/devel/unsub > > [michael.d.kinney@intel.com] > > -=3D-=3D-=3D-=3D-=3D-=3D > > > > -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109342): https://edk2.groups.io/g/devel/message/109342 Mute This Topic: https://groups.io/mt/101750274/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --000000000000e9c0630606f24479 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Mike,

I earlier assumed that both th= e fallthrough were intentional and just made it evident by writing the comm= ent. It was hence written actually for the code reader and not the static a= nalyzer. The comment was similar in nature and style to existing No/1-byte/= ... data comments.=C2=A0Incidentally, the comment satisfied the static a= nalyzer as well. It was beyond=C2=A0my expectation and I was pleasantly= surprised=C2=A0too. Without digging deep into the particular code and to a= void any functional code change,=C2=A0I just thought that's it.

Your comment forced me to come back specifically to = the particular code and to analyze it a bit more.

= My analysis now says that the fallthrough doesn't serve any real functi= onal purpose as there is absolutely no chance of=C2=A0if=C2= =A0blocks of the fallthrough=C2=A0case(s) being executed late= r and not executed in the original=C2=A0case:=C2=A0and hence = the fallthrough if it happens still leads=C2=A0to execution of=C2=A0r= eturn NULL;=C2=A0at the end of the function=C2=A0only. This = is better communicated by adding=C2=A0break<= /span>;=C2=A0statement in the original=C2=A0case:=C2= =A0and in a way, one may agree with the static analyzer that there was a=C2= =A0MISSING_BREAK. I should hence follow it = up with a v2 patch for this.

Ranbir Singh

On Thu, Oct 5, 2023 at 12:17=E2=80=AFAM Kinney, Michael D <michael.d.kinney@intel.com> wrot= e:
I do not pref= er special comments for one static analyzer.

Is there an alternative design/implementation of this code
to make it more readable and not trigger any static analysis
false positives?

Mike

> -----Original Message-----
> From: devel@= edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir
> Singh
> Sent: Tuesday, October 3, 2023 10:48 PM
> To: devel@ed= k2.groups.io; rsingh@ventanamicro.com
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Subject: [edk2-devel] [PATCH v1 2/2] MdeModulePkg/Bus/Usb/UsbMouseDxe:=
> Fix MISSING_BREAK Coverity issues
>
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>
> The function GetNextHidItem has a switch-case code in which the
> case 1: falls through to case 2: and then case 2: falls through
> to case 3: in the block.
>
> While this may be intentional, it is not evident to any general
> code reader as well as any static analyzer tool. Just adding
>
>=C2=A0 =C2=A0 =C2=A0// No break; here as this is an intentional fallthr= ough.
>
> as comment in between makes a reader as well as Coverity happy.
>
> REF: https://bugzilla.tianocore.org/show_b= ug.cgi?id=3D4222
>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Ray Ni <r= ay.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>=C2=A0 MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c | 8 ++++++++
>=C2=A0 1 file changed, 8 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> index acc19acd98e0..bc9a4824208b 100644
> --- a/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> +++ b/MdeModulePkg/Bus/Usb/UsbMouseDxe/MouseHid.c
> @@ -89,6 +89,10 @@ GetNextHidItem (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return StartPos;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
>
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 // No break; here as this is an intention= al fallthrough
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
> +
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 case 2:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // 2-byte data
>
> @@ -99,6 +103,10 @@ GetNextHidItem (
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return StartPos;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>
>
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 // No break; here as this is an intention= al fallthrough
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
> +
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 case 3:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 //
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 // 4-byte data, adjust size
>
> --
> 2.34.1
>
>
>
> -=3D-=3D-=3D-=3D-=3D-=3D
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#109309):
> https://edk2.groups.io/g/devel/message/109309<= br> > Mute This Topic: https://groups.io/mt/101750274/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [micha= el.d.kinney@intel.com]
> -=3D-=3D-=3D-=3D-=3D-=3D
>

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#109342) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--000000000000e9c0630606f24479--