When code is compiled in RELEASE mode, the ASSERT (Interval != 0); statement gets NULL'ified. Hence for Coverity it simply doesn't exist. Further, IMO Coverity seems to look at a function in isolation whether it is safe or not. It is however not necessary to fix all issues pointed out by Coverity if something looks to be a false positive or something is done deliberately. I will go by returning 1 and can still keep ASSERT if you prefer that way. I will put a change post ASSERT statement only, so that DEBUG mode still catches if wrongly '0' is sent. Should I do this or not - update the function header as well to reflect the minimum 'Interval' valid value as 1 and that if 0 is wrongly sent, it will be treated as 1 only in RELEASE mode ? On Mon, Aug 14, 2023 at 2:09 PM Wu, Hao A wrote: > My take is that: > > For all the possible calling scenario of UhciConvertPollRate() in current > > UhciDxe driver implementation, it is guaranteed that the input parameter > > 'Interval' will not be 0. > > > > I think this is why the "ASSERT (Interval != 0);" statement is put here to > > indicate such case will never happen and notify developers for future > changes > > in this driver that something is wrong. > > > > I do not know why Coverity is unable to figure this out. > > > > My personal preference is to return 1 (i.e. 1 << 0) as if the minimum > allowed > > value for 'Interval' (which is 1) is being passed into this > UhciConvertPollRate > > function if anything does go wrong brought by future changes. > > > > Best Regards, > > Hao Wu > > > > *From:* Ranbir Singh > *Sent:* Monday, August 14, 2023 3:49 PM > *To:* Wu, Hao A > *Cc:* devel@edk2.groups.io; Ni, Ray ; Veeresh Sangolli < > veeresh.sangolli@dellteam.com> > *Subject:* Re: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT > Coverity issue > > > > > > On Thu, Aug 10, 2023 at 8:09 AM Wu, Hao A wrote: > > > -----Original Message----- > > From: Ranbir Singh > > Sent: Monday, July 17, 2023 7:39 PM > > To: devel@edk2.groups.io; rsingh@ventanamicro.com > > Cc: Wu, Hao A ; Ni, Ray ; Veeresh > > Sangolli > > Subject: [PATCH v1 1/2] MdeModulePkg/Bus/Pci/UhciDxe: Fix BAD_SHIFT > > Coverity issue > > > > From: Ranbir Singh > > > > The function UhciConvertPollRate has a check > > > > ASSERT (Interval != 0); > > > > but this comes into play only in DEBUG mode. In Release mode, there is > > no handling if the Interval parameter value is ZERO. To avoid shifting > > by a negative amount later in the code flow in this undesirable case, > > it is better to handle it as well by simply returning ZERO. > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4211 > > > > Cc: Hao A Wu > > Cc: Ray Ni > > Co-authored-by: Veeresh Sangolli > > Signed-off-by: Ranbir Singh > > Signed-off-by: Ranbir Singh > > --- > > MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c > > b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c > > index c08f9496969b..8ddef4b68ccf 100644 > > --- a/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c > > +++ b/MdeModulePkg/Bus/Pci/UhciDxe/UhciSched.c > > @@ -214,6 +214,10 @@ UhciConvertPollRate ( > > > > > > ASSERT (Interval != 0); > > > > > > > > + if (Interval == 0) { > > > > + return 0; > > > Return 0 will cause further issues within UhciLinkQhToFrameList() & > UhciUnlinkQhFromFrameList() where the returned value is being used in the > below > 'for' statement: > > for (Index = 0; Index < UHCI_FRAME_NUM; Index += Qh->Interval) { > > > > One way is to prohibit UhciCreateQh() to proceed normally by checking if > Interval parameter value is 0 at the very beginning and may be add a DEBUG > statement and/or an ASSERT as well - return value then has to be NULL from > this function for this specific case of invalid Interval parameter value > being received as 0. That should take care of the issue Hao pointed above > in for loop. > > > > UhciCreateAsyncReq() may also need to introduce a Polling Interval > parameter value check similar to as it exists in > Uhci2AsyncInterruptTransfer() for a future direct call scenario. > > > > My thought is to return 1 (i.e. 1 << 0) as if the minimum allowed value for > 'Interval' (which is 1) is being passed into this UhciConvertPollRate > function. > > > > If we choose to modify UhciCreateQh(), then we may have status quo > in UhciConvertPollRate(). However, we can still pursue the return 1 (i.e. 1 > << 0) option here in which case I guess I should update the function header > as well to reflect the minimum 'Interval' valid value as 1 and that if 0 is > wrongly sent, it will be treated as 1 only. *When we do that in RELEASE > mode, should we still retain ASSERT ?* I think NO. > > > > If we choose to simply modify UhciConvertPollRate() as stated, then we > can have the status quo in UhciCreateQh() and UhciCreateAsyncReq(). > > > > > Best Regards, > Hao Wu > > > > > > + } > > > > + > > > > // > > > > // Find the index (1 based) of the highest non-zero bit > > > > // > > > > -- > > 2.34.1 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107730): https://edk2.groups.io/g/devel/message/107730 Mute This Topic: https://groups.io/mt/100212109/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-