Re: [edk2-devel] [PATCH 10/10] OvmfPkg/BasePciCapLib: suppress invalid "nullptr deref" warning

["Laszlo Ersek" <lersek@redhat.com>]

On 04/15/19 19:31, Philippe Mathieu-Daudé 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 == 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.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=1710
>> Issue: scan-0994.txt
>> Signed-off-by: Laszlo Ersek <lersek@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);

We'd have to #define STATIC_ANALYZER_HINT in "MdePkg/Include/Base.h",
somehow.

> 
> Regardless:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks! (For the other reviews too.)
Laszlo

> 
>> +
>>      InstanceZero->NumInstancesUnion.NumInstances++;
>>    }
>>    return RETURN_SUCCESS;
>>  
>>  DeletePciCapFromCapList:
>>    OrderedCollectionDelete (CapList->Capabilities, PciCapEntry, NULL);
>>  
>>  FreePciCap:
>>