Re: [edk2-devel] [PATCH 07/10] OvmfPkg/AcpiPlatformDxe: suppress invalid "deref of undef pointer" warning

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

On 4/13/19 1:31 AM, Laszlo Ersek wrote:
> RH covscan emits the following false positive:
> 
>> Error: CLANG_WARNING:
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:182:14:
>> warning: Dereference of undefined pointer value
>> #    Status = FwVol->ReadSection (
>> #             ^~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:164:7: note:
>> Assuming the condition is false
>> #  if (QemuDetected ()) {
>> #      ^~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:164:3: note:
>> Taking false branch
>> #  if (QemuDetected ()) {
>> #  ^
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:174:3: note:
>> Taking false branch
>> #  if (EFI_ERROR (Status)) {
>> #  ^
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:180:10: note:
>> Assuming 'Status' is equal to EFI_SUCCESS
>> #  while (Status == EFI_SUCCESS) {
>> #         ^~~~~~~~~~~~~~~~~~~~~
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:180:3: note:
>> Loop condition is true.  Entering loop body
>> #  while (Status == EFI_SUCCESS) {
>> #  ^
>> edk2-89910a39dcfd/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c:182:14: note:
>> Dereference of undefined pointer value
>> #    Status = FwVol->ReadSection (
>> #             ^~~~~~~~~~~~~~~~~~
>> #  180|     while (Status == EFI_SUCCESS) {
>> #  181|
>> #  182|->     Status = FwVol->ReadSection (
>> #  183|                         FwVol,
>> #  184|                         (EFI_GUID*)PcdGetPtr (PcdAcpiTableStorageFile),
> 
> This is invalid because LocateFvInstanceWithTables() sets FwVol on
> success.
> 
> Suppress the message by:
> - assigning FwVol NULL first (this would replace the original report with
>   "nullptr deref"),
> - asserting that FwVol is no longer NULL, on success.
> 
> What's important here is that ASSERT() ends with ANALYZER_UNREACHABLE() on
> failure.
> 
> 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-0991.txt
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> index f2c49953950b..2b529d58a15c 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> @@ -156,23 +156,30 @@ InstallOvmfFvTables (
>    TableHandle  = 0;
>  
>    if (QemuDetected ()) {
>      TableInstallFunction = QemuInstallAcpiTable;
>    } else {
>      TableInstallFunction = InstallAcpiTable;
>    }
>  
> +  //
> +  // set FwVol (and use an ASSERT() below) to suppress incorrect
> +  // compiler/analyzer warnings
> +  //
> +  FwVol = NULL;
>    //
>    // Locate the firmware volume protocol
>    //
>    Status = LocateFvInstanceWithTables (&FwVol);
>    if (EFI_ERROR (Status)) {
>      return EFI_ABORTED;
>    }
> +  ASSERT (FwVol != NULL);
> +
>    //
>    // Read tables from the storage file.
>    //
>    while (Status == EFI_SUCCESS) {
>  
>      Status = FwVol->ReadSection (
>                        FwVol,
>                        (EFI_GUID*)PcdGetPtr (PcdAcpiTableStorageFile),
> 

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