OvmfPkg: QemuVideoDxe: tidy up error checking/handling in & under Start()

In QemuVideoControllerDriverStart():
- remove redundant zero-initialization of:
  - Private->Handle (2 locations)
  - Private->GopDevicePath (when at devpath end)

- remove fields used for error handling only:
  - PciAttributesSaved

- tigthen scope of temporaries:
  - MmioDesc
  - AcpiDeviceNode

- supplement missing error checks:
  - AppendDevicePathNode() can fail with out-of-memory (2 locations)
  - when installing GopDevicePath
  - retval of QemuVideoGraphicsOutputConstructor() (can justifiedly fail
    with out-of-resources)

- plug leaks on error:
  - free GopDevicePath (AppendDevicePathNode() allocates dynamically)
  - uninstall GopDevicePath
  - free Private->ModeData
  - call QemuVideoGraphicsOutputDestructor()
  - uninstall GOP

In QemuVideoGraphicsOutputConstructor(), called by Start():
- supplement missing error checks:
  - QemuVideoGraphicsOutputSetMode() retval (it can fail with
    out-of-resources)

- plug leaks on error:
  - free Mode->Info
  - free Mode

In QemuVideoCirrusModeSetup() and QemuVideoBochsModeSetup(), both called
by Start():
- supplement missing error checks:
  - AllocatePool() can fail in both

In QemuVideoGraphicsOutputDestructor(), called by Start() on the error
path:
- plug leaks:
  - free Private->LineBuffer, which is allocated in
    Start() -> Constructor() -> SetMode()

In QemuVideoGraphicsOutputSetMode(), called by Start() indirectly:
- remove redundant zero-assignment to:
  - Private->LineBuffer

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@15282 6f19259b-4bc3-4df7-8a09-765794883524

3 files changed, 129 insertions, 95 deletions

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index b253ec734e..d7716ce6a9 100644
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -203,29 +203,23 @@ QemuVideoControllerDriverStart (
 {

   EFI_STATUS                        Status;

   QEMU_VIDEO_PRIVATE_DATA           *Private;

-  BOOLEAN                           PciAttributesSaved;

   EFI_DEVICE_PATH_PROTOCOL          *ParentDevicePath;

-  ACPI_ADR_DEVICE_PATH              AcpiDeviceNode;

   PCI_TYPE00                        Pci;

   QEMU_VIDEO_CARD                   *Card;

-  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc;

   EFI_PCI_IO_PROTOCOL               *ChildPciIo;

 

-  PciAttributesSaved = FALSE;

   //

   // Allocate Private context data for GOP inteface.

   //

   Private = AllocateZeroPool (sizeof (QEMU_VIDEO_PRIVATE_DATA));

   if (Private == NULL) {

-    Status = EFI_OUT_OF_RESOURCES;

-    goto Error;

+    return EFI_OUT_OF_RESOURCES;

   }

 

   //

   // Set up context record

   //

   Private->Signature  = QEMU_VIDEO_PRIVATE_DATA_SIGNATURE;

-  Private->Handle     = NULL;

 

   //

   // Open PCI I/O Protocol

@@ -239,7 +233,7 @@ QemuVideoControllerDriverStart (
                   EFI_OPEN_PROTOCOL_BY_DRIVER

                   );

   if (EFI_ERROR (Status)) {

-    goto Error;

+    goto FreePrivate;

   }

 

   //

@@ -253,13 +247,16 @@ QemuVideoControllerDriverStart (
                         &Pci

                         );

   if (EFI_ERROR (Status)) {

-    goto Error;

+    goto ClosePciIo;

   }

 

+  //

+  // Determine card variant.

+  //

   Card = QemuVideoDetect(Pci.Hdr.VendorId, Pci.Hdr.DeviceId);

   if (Card == NULL) {

     Status = EFI_DEVICE_ERROR;

-    goto Error;

+    goto ClosePciIo;

   }

   Private->Variant = Card->Variant;

 

@@ -274,10 +271,12 @@ QemuVideoControllerDriverStart (
                     );

 

   if (EFI_ERROR (Status)) {

-    goto Error;

+    goto ClosePciIo;

   }

-  PciAttributesSaved = TRUE;

 

+  //

+  // Set new PCI attributes

+  //

   Status = Private->PciIo->Attributes (

                             Private->PciIo,

                             EfiPciIoAttributeOperationEnable,

@@ -285,13 +284,15 @@ QemuVideoControllerDriverStart (
                             NULL

                             );

   if (EFI_ERROR (Status)) {

-    goto Error;

+    goto ClosePciIo;

   }

 

   //

   // Check whenever the qemu stdvga mmio bar is present (qemu 1.3+).

   //

   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) {

+    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc;

+

     Status = Private->PciIo->GetBarAttributes (

                         Private->PciIo,

                         PCI_BAR_IDX2,

@@ -322,7 +323,7 @@ QemuVideoControllerDriverStart (
     if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {

       DEBUG ((EFI_D_INFO, "QemuVideo: BochsID mismatch (got 0x%x)\n", BochsId));

       Status = EFI_DEVICE_ERROR;

-      goto Error;

+      goto RestoreAttributes;

     }

   }

 

@@ -335,13 +336,15 @@ QemuVideoControllerDriverStart (
                   (VOID **) &ParentDevicePath

                   );

   if (EFI_ERROR (Status)) {

-    goto Error;

+    goto RestoreAttributes;

   }

 

   //

   // Set Gop Device Path

   //

   if (RemainingDevicePath == NULL) {

+    ACPI_ADR_DEVICE_PATH AcpiDeviceNode;

+

     ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));

     AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;

     AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;

@@ -352,31 +355,37 @@ QemuVideoControllerDriverStart (
                                         ParentDevicePath,

                                         (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode

                                         );

+    if (Private->GopDevicePath == NULL) {

+      Status = EFI_OUT_OF_RESOURCES;

+      goto RestoreAttributes;

+    }

   } else if (!IsDevicePathEnd (RemainingDevicePath)) {

     //

     // If RemainingDevicePath isn't the End of Device Path Node, 

     // only scan the specified device by RemainingDevicePath

     //

     Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath, RemainingDevicePath);

-  } else {

-    //

-    // If RemainingDevicePath is the End of Device Path Node, 

-    // don't create child device and return EFI_SUCCESS

-    //

-    Private->GopDevicePath = NULL;

+    if (Private->GopDevicePath == NULL) {

+      Status = EFI_OUT_OF_RESOURCES;

+      goto RestoreAttributes;

+    }

   }

-    

+

+  //

+  // Create new child handle and install the device path protocol on it only if

+  // RemainingDevicePath equals NULL, or doesn't point to the End of Device

+  // Path Node.

+  //

   if (Private->GopDevicePath != NULL) {

-    //

-    // Creat child handle and device path protocol firstly

-    //

-    Private->Handle = NULL;

     Status = gBS->InstallMultipleProtocolInterfaces (

                     &Private->Handle,

                     &gEfiDevicePathProtocolGuid,

                     Private->GopDevicePath,

                     NULL

                     );

+    if (EFI_ERROR (Status)) {

+      goto FreeGopDevicePath;

+    }

   }

 

   //

@@ -397,84 +406,86 @@ QemuVideoControllerDriverStart (
     break;

   }

   if (EFI_ERROR (Status)) {

-    goto Error;

+    goto UninstallGopDevicePath;

   }

 

+  //

+  // If RemainingDevicePath points to the End of Device Path Node, then we

+  // haven't created a child handle, and we're done.

+  //

   if (Private->GopDevicePath == NULL) {

-    //

-    // If RemainingDevicePath is the End of Device Path Node, 

-    // don't create child device and return EFI_SUCCESS

-    //

-    Status = EFI_SUCCESS;

-  } else {

-

-    //

-    // Start the GOP software stack.

-    //

-    Status = QemuVideoGraphicsOutputConstructor (Private);

-    ASSERT_EFI_ERROR (Status);

+    return EFI_SUCCESS;

+  }

 

-    Status = gBS->InstallMultipleProtocolInterfaces (

-                    &Private->Handle,

-                    &gEfiGraphicsOutputProtocolGuid,

-                    &Private->GraphicsOutput,

-                    NULL

-                    );

-    if (EFI_ERROR (Status)) {

-      goto Error;

-    }

+  //

+  // Start the GOP software stack.

+  //

+  Status = QemuVideoGraphicsOutputConstructor (Private);

+  if (EFI_ERROR (Status)) {

+    goto FreeModeData;

+  }

 

-    Status = gBS->OpenProtocol (

-                  Controller,

-                  &gEfiPciIoProtocolGuid,

-                  (VOID **) &ChildPciIo,

-                  This->DriverBindingHandle,

-                  Private->Handle,

-                  EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER

+  Status = gBS->InstallMultipleProtocolInterfaces (

+                  &Private->Handle,

+                  &gEfiGraphicsOutputProtocolGuid,

+                  &Private->GraphicsOutput,

+                  NULL

                   );

-

-    if (EFI_ERROR (Status)) {

-      goto Error;

-    }

+  if (EFI_ERROR (Status)) {

+    goto DestructQemuVideoGraphics;

   }

 

-Error:

+  //

+  // Reference parent handle from child handle.

+  //

+  Status = gBS->OpenProtocol (

+                Controller,

+                &gEfiPciIoProtocolGuid,

+                (VOID **) &ChildPciIo,

+                This->DriverBindingHandle,

+                Private->Handle,

+                EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER

+                );

   if (EFI_ERROR (Status)) {

-    if (Private) {

-      if (Private->PciIo) {

-        if (PciAttributesSaved == TRUE) {

-          //

-          // Restore original PCI attributes

-          //

-          Private->PciIo->Attributes (

-                          Private->PciIo,

-                          EfiPciIoAttributeOperationSet,

-                          Private->OriginalPciAttributes,

-                          NULL

-                          );

-        }

-        //

-        // Close the PCI I/O Protocol

-        //

-        gBS->CloseProtocol (

-              Controller,

-              &gEfiPciIoProtocolGuid,

-              This->DriverBindingHandle,

-              Controller

-              );

-

-        gBS->CloseProtocol (

-              Controller,

-              &gEfiPciIoProtocolGuid,

-              This->DriverBindingHandle,

-              Private->Handle

-              );

-      }

+    goto UninstallGop;

+  }

 

-      gBS->FreePool (Private);

-    }

+  return EFI_SUCCESS;

+

+UninstallGop:

+  gBS->UninstallProtocolInterface (Private->Handle,

+         &gEfiGraphicsOutputProtocolGuid, &Private->GraphicsOutput);

+

+DestructQemuVideoGraphics:

+  QemuVideoGraphicsOutputDestructor (Private);

+

+FreeModeData:

+  FreePool (Private->ModeData);

+

+UninstallGopDevicePath:

+  //

+  // Handles the case transparently when Private->Handle and

+  // Private->GopDevicePath are NULL.

+  //

+  gBS->UninstallProtocolInterface (Private->Handle,

+         &gEfiDevicePathProtocolGuid, Private->GopDevicePath);

+

+FreeGopDevicePath:

+  if (Private->GopDevicePath != NULL) {

+    FreePool (Private->GopDevicePath);

   }

 

+RestoreAttributes:

+  Private->PciIo->Attributes (Private->PciIo, EfiPciIoAttributeOperationSet,

+                    Private->OriginalPciAttributes, NULL);

+

+ClosePciIo:

+  gBS->CloseProtocol (Controller, &gEfiPciIoProtocolGuid,

+         This->DriverBindingHandle, Controller);

+

+FreePrivate:

+  FreePool (Private);

+

   return Status;

 }

 

diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 30aac7f95f..1b7db329b9 100644
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -176,7 +176,6 @@ Routine Description:
     gBS->FreePool (Private->LineBuffer);

   }

 

-  Private->LineBuffer = NULL;

   Private->LineBuffer = AllocatePool (4 * ModeData->HorizontalResolution);

   if (Private->LineBuffer == NULL) {

     return EFI_OUT_OF_RESOURCES;

@@ -321,13 +320,14 @@ QemuVideoGraphicsOutputConstructor (
   if (EFI_ERROR (Status)) {

     return Status;

   }

+

   Status = gBS->AllocatePool (

                   EfiBootServicesData,

                   sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),

                   (VOID **) &Private->GraphicsOutput.Mode->Info

                   );

   if (EFI_ERROR (Status)) {

-    return Status;

+    goto FreeMode;

   }

   Private->GraphicsOutput.Mode->MaxMode = (UINT32) Private->MaxMode;

   Private->GraphicsOutput.Mode->Mode    = GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER;

@@ -337,7 +337,11 @@ QemuVideoGraphicsOutputConstructor (
   //

   // Initialize the hardware

   //

-  GraphicsOutput->SetMode (GraphicsOutput, 0);

+  Status = GraphicsOutput->SetMode (GraphicsOutput, 0);

+  if (EFI_ERROR (Status)) {

+    goto FreeInfo;

+  }

+

   DrawLogo (

     Private,

     Private->ModeData[Private->GraphicsOutput.Mode->Mode].HorizontalResolution,

@@ -345,6 +349,15 @@ QemuVideoGraphicsOutputConstructor (
     );

 

   return EFI_SUCCESS;

+

+FreeInfo:

+  FreePool (Private->GraphicsOutput.Mode->Info);

+

+FreeMode:

+  FreePool (Private->GraphicsOutput.Mode);

+  Private->GraphicsOutput.Mode = NULL;

+

+  return Status;

 }

 

 EFI_STATUS

@@ -363,6 +376,10 @@ Returns:
 

 --*/

 {

+  if (Private->LineBuffer != NULL) {

+    FreePool (Private->LineBuffer);

+  }

+

   if (Private->GraphicsOutput.Mode != NULL) {

     if (Private->GraphicsOutput.Mode->Info != NULL) {

       gBS->FreePool (Private->GraphicsOutput.Mode->Info);

diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index 305797bd50..37744786cb 100644
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -176,6 +176,9 @@ QemuVideoCirrusModeSetup (
   Private->ModeData = AllocatePool (

                         sizeof (Private->ModeData[0]) * QEMU_VIDEO_CIRRUS_MODE_COUNT

                         );

+  if (Private->ModeData == NULL) {

+    return EFI_OUT_OF_RESOURCES;

+  }

   ModeData = Private->ModeData;

   VideoMode = &QemuVideoCirrusModes[0];

   for (Index = 0; Index < QEMU_VIDEO_CIRRUS_MODE_COUNT; Index ++) {

@@ -228,6 +231,9 @@ QemuVideoBochsModeSetup (
   Private->ModeData = AllocatePool (

                         sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT

                         );

+  if (Private->ModeData == NULL) {

+    return EFI_OUT_OF_RESOURCES;

+  }

   ModeData = Private->ModeData;

   VideoMode = &QemuVideoBochsModes[0];

   for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {