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

2019-04-16 Thread Laszlo Ersek 
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//
>>257Status = OrderedCollectionInsert (CapList->Capabilities, 
>> &

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

2019-04-15 Thread Philippe Mathieu-Daudé 
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//
>257Status = OrderedCollectionInsert (CapList->Capabilities, 
> &PciCapEntry,
>258   PciCap);
>259if (RETURN_ERROR (Status)) {
> 
> That assumption is incorrect.)
> 
> Add a comment and an ASSERT().
>