From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 16 Apr 2019 04:01:28 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EB5D53082137; Tue, 16 Apr 2019 11:01:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-171.rdu2.redhat.com [10.10.120.171]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8B32A608A4; Tue, 16 Apr 2019 11:01:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 10/10] OvmfPkg/BasePciCapLib: suppress invalid "nullptr deref" warning To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io Cc: Ard Biesheuvel , Jordan Justen References: <20190412233128.4756-1-lersek@redhat.com> <20190412233128.4756-11-lersek@redhat.com> <98bcdd29-6df6-b13e-77df-a59b07555d8d@redhat.com> From: "Laszlo Ersek" Message-ID: <779ba225-6b82-911b-8cf3-186da186aa78@redhat.com> Date: Tue, 16 Apr 2019 13:01:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <98bcdd29-6df6-b13e-77df-a59b07555d8d@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Tue, 16 Apr 2019 11:01:28 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 04/15/19 19:31, Philippe Mathieu-Daud=C3=A9 wrote: > On 4/13/19 1:31 AM, Laszlo Ersek wrote: >> RH covscan reports the following "nullptr deref" warning: >> >>> Error: CLANG_WARNING: >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:312:5= : >>> warning: Dereference of null pointer >>> # InstanceZero->NumInstancesUnion.NumInstances++; >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:7= : >>> note: Assuming 'OutCapList' is not equal to NULL >>> # if (OutCapList =3D=3D NULL) { >>> # ^~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:3= : >>> note: Taking false branch >>> # if (OutCapList =3D=3D NULL) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:7= : >>> note: Assuming the condition is false >>> # if (OutCapList->Capabilities =3D=3D NULL) { >>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:3= : >>> note: Taking false branch >>> # if (OutCapList->Capabilities =3D=3D NULL) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:7= : >>> note: Assuming 'CapHdrOffsets' is not equal to NULL >>> # if (CapHdrOffsets =3D=3D NULL) { >>> # ^~~~~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:3= : >>> note: Taking false branch >>> # if (CapHdrOffsets =3D=3D NULL) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:546:3= : >>> note: Taking false branch >>> # if (RETURN_ERROR (Status)) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:7= : >>> note: Assuming the condition is true >>> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) !=3D 0) { >>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:3= : >>> note: Taking true branch >>> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) !=3D 0) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:557:5= : >>> note: Taking false branch >>> # if (RETURN_ERROR (Status)) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:565:1= 2: >>> note: Assuming 'NormalCapHdrOffset' is > 0 >>> # while (NormalCapHdrOffset > 0) { >>> # ^~~~~~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:565:5= : >>> note: Loop condition is true. Entering loop body >>> # while (NormalCapHdrOffset > 0) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:570:7= : >>> note: Taking false branch >>> # if (RETURN_ERROR (Status)) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:574:1= 6: >>> note: Calling 'InsertPciCap' >>> # Status =3D InsertPciCap (OutCapList, CapHdrOffsets, PciCapNorm= al, >>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~= ~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:235:3= : >>> note: Null pointer value stored to 'InstanceZero' >>> # InstanceZero =3D NULL; >>> # ^~~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:7= : >>> note: Assuming 'PciCap' is not equal to NULL >>> # if (PciCap =3D=3D NULL) { >>> # ^~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:3= : >>> note: Taking false branch >>> # if (PciCap =3D=3D NULL) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:259:3= : >>> note: Taking false branch >>> # if (RETURN_ERROR (Status)) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:297:3= : >>> note: Taking false branch >>> # if (RETURN_ERROR (Status)) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:311:7= : >>> note: Assuming the condition is true >>> # if (PciCap->Key.Instance > 0) { >>> # ^~~~~~~~~~~~~~~~~~~~~~~~ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:311:3= : >>> note: Taking true branch >>> # if (PciCap->Key.Instance > 0) { >>> # ^ >>> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:312:5= : >>> note: Dereference of null pointer >>> # InstanceZero->NumInstancesUnion.NumInstances++; >>> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> # 310| // >>> # 311| if (PciCap->Key.Instance > 0) { >>> # 312|-> InstanceZero->NumInstancesUnion.NumInstances++; >>> # 313| } >>> # 314| return RETURN_SUCCESS; >> >> The warning is invalid: the flagged dereferencing of "InstanceZero" is >> gated by a condition that is only satisfied if we dereference >> "InstanceZero" *first*. >> >> (Perhaps the analyzer assumes that the OrderedCollectionInsert() call, >> just before line 259, can change the value of "PciCap->Key.Instance" v= ia >> the last argument: >> >> 254 // >> 255 // Add PciCap to CapList. >> 256 // >> 257 Status =3D OrderedCollectionInsert (CapList->Capabilities, &= PciCapEntry, >> 258 PciCap); >> 259 if (RETURN_ERROR (Status)) { >> >> That assumption is incorrect.) >> >> Add a comment and an ASSERT(). >> >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1710 >> Issue: scan-0994.txt >> Signed-off-by: Laszlo Ersek >> --- >> OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c b/OvmfPkg/L= ibrary/BasePciCapLib/BasePciCapLib.c >> index 4968d2b478db..c6f2c726509f 100644 >> --- a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c >> +++ b/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c >> @@ -298,16 +298,23 @@ InsertPciCap ( >> goto DeletePciCapFromCapList; >> } >> =20 >> // >> // Now we can bump the instance count maintained in Instance#0, if = PciCap is >> // not the first instance of (Domain, CapId). >> // >> if (PciCap->Key.Instance > 0) { >> + // >> + // Suppress invalid "nullptr dereference" compiler/analyzer warni= ngs: the >> + // only way for "PciCap->Key.Instance" to be positive here is for= it to >> + // have been assigned *from* dereferencing "InstanceZero" above. >> + // >> + ASSERT (InstanceZero !=3D NULL); >=20 > What about adding a STATIC_ANALYZER_HINT() only defined by covscan? >=20 > STATIC_ANALYZER_HINT (InstanceZero !=3D NULL); We'd have to #define STATIC_ANALYZER_HINT in "MdePkg/Include/Base.h", somehow. >=20 > Regardless: > Reviewed-by: Philippe Mathieu-Daude Thanks! (For the other reviews too.) Laszlo >=20 >> + >> InstanceZero->NumInstancesUnion.NumInstances++; >> } >> return RETURN_SUCCESS; >> =20 >> DeletePciCapFromCapList: >> OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL); >> =20 >> FreePciCap: >>