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 == NULL) { >> # ^~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:509:3: >> note: Taking false branch >> # if (OutCapList == NULL) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:7: >> note: Assuming the condition is false >> # if (OutCapList->Capabilities == NULL) { >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:518:3: >> note: Taking false branch >> # if (OutCapList->Capabilities == NULL) { >> # ^ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:7: >> note: Assuming 'CapHdrOffsets' is not equal to NULL >> # if (CapHdrOffsets == NULL) { >> # ^~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:529:3: >> note: Taking false branch >> # if (CapHdrOffsets == 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) != 0) { >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:549:3: >> note: Taking true branch >> # if ((PciStatusReg & EFI_PCI_STATUS_CAPABILITY) != 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:12: >> 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:16: >> note: Calling 'InsertPciCap' >> # Status = InsertPciCap (OutCapList, CapHdrOffsets, PciCapNormal, >> # ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:235:3: >> note: Null pointer value stored to 'InstanceZero' >> # InstanceZero = NULL; >> # ^~~~~~~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:7: >> note: Assuming 'PciCap' is not equal to NULL >> # if (PciCap == NULL) { >> # ^~~~~~~~~~~~~~ >> edk2-89910a39dcfd/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c:243:3: >> note: Taking false branch >> # if (PciCap == 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" via > the last argument: > > 254 // > 255 // Add PciCap to CapList. > 256 // > 257 Status = OrderedCollectionInsert (CapList->Capabilities, > &PciCapEntry, > 258 PciCap); > 259 if (RETURN_ERROR (Status)) { > > That assumption is incorrect.) > > Add a comment and an ASSERT(). > > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710 > Issue: scan-0994.txt > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/OvmfPkg/Library/BasePciCapLib/BasePciCapLib.c > b/OvmfPkg/Library/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; > } > > // > // 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 warnings: the > + // only way for "PciCap->Key.Instance" to be positive here is for it to > + // have been assigned *from* dereferencing "InstanceZero" above. > + // > + ASSERT (InstanceZero != NULL);
What about adding a STATIC_ANALYZER_HINT() only defined by covscan? STATIC_ANALYZER_HINT (InstanceZero != NULL); Regardless: Reviewed-by: Philippe Mathieu-Daude <phi...@redhat.com> > + > InstanceZero->NumInstancesUnion.NumInstances++; > } > return RETURN_SUCCESS; > > DeletePciCapFromCapList: > OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL); > > FreePciCap: > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39109): https://edk2.groups.io/g/devel/message/39109 Mute This Topic: https://groups.io/mt/31070310/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-