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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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);

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

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