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

diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c
index b253ec734edf9e214a916ad58930622f6a81124e..d7716ce6a9df969b259fa096bca4215c926a7aa6 100644 (file)
--- a/OvmfPkg/QemuVideoDxe/Driver.c
+++ b/OvmfPkg/QemuVideoDxe/Driver.c
@@ -203,29 +203,23 @@ QemuVideoControllerDriverStart (
 {\r
   EFI_STATUS                        Status;\r
   QEMU_VIDEO_PRIVATE_DATA           *Private;\r
-  BOOLEAN                           PciAttributesSaved;\r
   EFI_DEVICE_PATH_PROTOCOL          *ParentDevicePath;\r
-  ACPI_ADR_DEVICE_PATH              AcpiDeviceNode;\r
   PCI_TYPE00                        Pci;\r
   QEMU_VIDEO_CARD                   *Card;\r
-  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc;\r
   EFI_PCI_IO_PROTOCOL               *ChildPciIo;\r
 \r
-  PciAttributesSaved = FALSE;\r
   //\r
   // Allocate Private context data for GOP inteface.\r
   //\r
   Private = AllocateZeroPool (sizeof (QEMU_VIDEO_PRIVATE_DATA));\r
   if (Private == NULL) {\r
-    Status = EFI_OUT_OF_RESOURCES;\r
-    goto Error;\r
+    return EFI_OUT_OF_RESOURCES;\r
   }\r
 \r
   //\r
   // Set up context record\r
   //\r
   Private->Signature  = QEMU_VIDEO_PRIVATE_DATA_SIGNATURE;\r
-  Private->Handle     = NULL;\r
 \r
   //\r
   // Open PCI I/O Protocol\r
@@ -239,7 +233,7 @@ QemuVideoControllerDriverStart (
                   EFI_OPEN_PROTOCOL_BY_DRIVER\r
                   );\r
   if (EFI_ERROR (Status)) {\r
-    goto Error;\r
+    goto FreePrivate;\r
   }\r
 \r
   //\r
@@ -253,13 +247,16 @@ QemuVideoControllerDriverStart (
                         &Pci\r
                         );\r
   if (EFI_ERROR (Status)) {\r
-    goto Error;\r
+    goto ClosePciIo;\r
   }\r
 \r
+  //\r
+  // Determine card variant.\r
+  //\r
   Card = QemuVideoDetect(Pci.Hdr.VendorId, Pci.Hdr.DeviceId);\r
   if (Card == NULL) {\r
     Status = EFI_DEVICE_ERROR;\r
-    goto Error;\r
+    goto ClosePciIo;\r
   }\r
   Private->Variant = Card->Variant;\r
 \r
@@ -274,10 +271,12 @@ QemuVideoControllerDriverStart (
                     );\r
 \r
   if (EFI_ERROR (Status)) {\r
-    goto Error;\r
+    goto ClosePciIo;\r
   }\r
-  PciAttributesSaved = TRUE;\r
 \r
+  //\r
+  // Set new PCI attributes\r
+  //\r
   Status = Private->PciIo->Attributes (\r
                             Private->PciIo,\r
                             EfiPciIoAttributeOperationEnable,\r
@@ -285,13 +284,15 @@ QemuVideoControllerDriverStart (
                             NULL\r
                             );\r
   if (EFI_ERROR (Status)) {\r
-    goto Error;\r
+    goto ClosePciIo;\r
   }\r
 \r
   //\r
   // Check whenever the qemu stdvga mmio bar is present (qemu 1.3+).\r
   //\r
   if (Private->Variant == QEMU_VIDEO_BOCHS_MMIO) {\r
+    EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *MmioDesc;\r
+\r
     Status = Private->PciIo->GetBarAttributes (\r
                         Private->PciIo,\r
                         PCI_BAR_IDX2,\r
@@ -322,7 +323,7 @@ QemuVideoControllerDriverStart (
     if ((BochsId & 0xFFF0) != VBE_DISPI_ID0) {\r
       DEBUG ((EFI_D_INFO, "QemuVideo: BochsID mismatch (got 0x%x)\n", BochsId));\r
       Status = EFI_DEVICE_ERROR;\r
-      goto Error;\r
+      goto RestoreAttributes;\r
     }\r
   }\r
 \r
@@ -335,13 +336,15 @@ QemuVideoControllerDriverStart (
                   (VOID **) &ParentDevicePath\r
                   );\r
   if (EFI_ERROR (Status)) {\r
-    goto Error;\r
+    goto RestoreAttributes;\r
   }\r
 \r
   //\r
   // Set Gop Device Path\r
   //\r
   if (RemainingDevicePath == NULL) {\r
+    ACPI_ADR_DEVICE_PATH AcpiDeviceNode;\r
+\r
     ZeroMem (&AcpiDeviceNode, sizeof (ACPI_ADR_DEVICE_PATH));\r
     AcpiDeviceNode.Header.Type = ACPI_DEVICE_PATH;\r
     AcpiDeviceNode.Header.SubType = ACPI_ADR_DP;\r
@@ -352,31 +355,37 @@ QemuVideoControllerDriverStart (
                                         ParentDevicePath,\r
                                         (EFI_DEVICE_PATH_PROTOCOL *) &AcpiDeviceNode\r
                                         );\r
+    if (Private->GopDevicePath == NULL) {\r
+      Status = EFI_OUT_OF_RESOURCES;\r
+      goto RestoreAttributes;\r
+    }\r
   } else if (!IsDevicePathEnd (RemainingDevicePath)) {\r
     //\r
     // If RemainingDevicePath isn't the End of Device Path Node, \r
     // only scan the specified device by RemainingDevicePath\r
     //\r
     Private->GopDevicePath = AppendDevicePathNode (ParentDevicePath, RemainingDevicePath);\r
-  } else {\r
-    //\r
-    // If RemainingDevicePath is the End of Device Path Node, \r
-    // don't create child device and return EFI_SUCCESS\r
-    //\r
-    Private->GopDevicePath = NULL;\r
+    if (Private->GopDevicePath == NULL) {\r
+      Status = EFI_OUT_OF_RESOURCES;\r
+      goto RestoreAttributes;\r
+    }\r
   }\r
-    \r
+\r
+  //\r
+  // Create new child handle and install the device path protocol on it only if\r
+  // RemainingDevicePath equals NULL, or doesn't point to the End of Device\r
+  // Path Node.\r
+  //\r
   if (Private->GopDevicePath != NULL) {\r
-    //\r
-    // Creat child handle and device path protocol firstly\r
-    //\r
-    Private->Handle = NULL;\r
     Status = gBS->InstallMultipleProtocolInterfaces (\r
                     &Private->Handle,\r
                     &gEfiDevicePathProtocolGuid,\r
                     Private->GopDevicePath,\r
                     NULL\r
                     );\r
+    if (EFI_ERROR (Status)) {\r
+      goto FreeGopDevicePath;\r
+    }\r
   }\r
 \r
   //\r
@@ -397,84 +406,86 @@ QemuVideoControllerDriverStart (
     break;\r
   }\r
   if (EFI_ERROR (Status)) {\r
-    goto Error;\r
+    goto UninstallGopDevicePath;\r
   }\r
 \r
+  //\r
+  // If RemainingDevicePath points to the End of Device Path Node, then we\r
+  // haven't created a child handle, and we're done.\r
+  //\r
   if (Private->GopDevicePath == NULL) {\r
-    //\r
-    // If RemainingDevicePath is the End of Device Path Node, \r
-    // don't create child device and return EFI_SUCCESS\r
-    //\r
-    Status = EFI_SUCCESS;\r
-  } else {\r
-\r
-    //\r
-    // Start the GOP software stack.\r
-    //\r
-    Status = QemuVideoGraphicsOutputConstructor (Private);\r
-    ASSERT_EFI_ERROR (Status);\r
+    return EFI_SUCCESS;\r
+  }\r
 \r
-    Status = gBS->InstallMultipleProtocolInterfaces (\r
-                    &Private->Handle,\r
-                    &gEfiGraphicsOutputProtocolGuid,\r
-                    &Private->GraphicsOutput,\r
-                    NULL\r
-                    );\r
-    if (EFI_ERROR (Status)) {\r
-      goto Error;\r
-    }\r
+  //\r
+  // Start the GOP software stack.\r
+  //\r
+  Status = QemuVideoGraphicsOutputConstructor (Private);\r
+  if (EFI_ERROR (Status)) {\r
+    goto FreeModeData;\r
+  }\r
 \r
-    Status = gBS->OpenProtocol (\r
-                  Controller,\r
-                  &gEfiPciIoProtocolGuid,\r
-                  (VOID **) &ChildPciIo,\r
-                  This->DriverBindingHandle,\r
-                  Private->Handle,\r
-                  EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER\r
+  Status = gBS->InstallMultipleProtocolInterfaces (\r
+                  &Private->Handle,\r
+                  &gEfiGraphicsOutputProtocolGuid,\r
+                  &Private->GraphicsOutput,\r
+                  NULL\r
                   );\r
-\r
-    if (EFI_ERROR (Status)) {\r
-      goto Error;\r
-    }\r
+  if (EFI_ERROR (Status)) {\r
+    goto DestructQemuVideoGraphics;\r
   }\r
 \r
-Error:\r
+  //\r
+  // Reference parent handle from child handle.\r
+  //\r
+  Status = gBS->OpenProtocol (\r
+                Controller,\r
+                &gEfiPciIoProtocolGuid,\r
+                (VOID **) &ChildPciIo,\r
+                This->DriverBindingHandle,\r
+                Private->Handle,\r
+                EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER\r
+                );\r
   if (EFI_ERROR (Status)) {\r
-    if (Private) {\r
-      if (Private->PciIo) {\r
-        if (PciAttributesSaved == TRUE) {\r
-          //\r
-          // Restore original PCI attributes\r
-          //\r
-          Private->PciIo->Attributes (\r
-                          Private->PciIo,\r
-                          EfiPciIoAttributeOperationSet,\r
-                          Private->OriginalPciAttributes,\r
-                          NULL\r
-                          );\r
-        }\r
-        //\r
-        // Close the PCI I/O Protocol\r
-        //\r
-        gBS->CloseProtocol (\r
-              Controller,\r
-              &gEfiPciIoProtocolGuid,\r
-              This->DriverBindingHandle,\r
-              Controller\r
-              );\r
-\r
-        gBS->CloseProtocol (\r
-              Controller,\r
-              &gEfiPciIoProtocolGuid,\r
-              This->DriverBindingHandle,\r
-              Private->Handle\r
-              );\r
-      }\r
+    goto UninstallGop;\r
+  }\r
 \r
-      gBS->FreePool (Private);\r
-    }\r
+  return EFI_SUCCESS;\r
+\r
+UninstallGop:\r
+  gBS->UninstallProtocolInterface (Private->Handle,\r
+         &gEfiGraphicsOutputProtocolGuid, &Private->GraphicsOutput);\r
+\r
+DestructQemuVideoGraphics:\r
+  QemuVideoGraphicsOutputDestructor (Private);\r
+\r
+FreeModeData:\r
+  FreePool (Private->ModeData);\r
+\r
+UninstallGopDevicePath:\r
+  //\r
+  // Handles the case transparently when Private->Handle and\r
+  // Private->GopDevicePath are NULL.\r
+  //\r
+  gBS->UninstallProtocolInterface (Private->Handle,\r
+         &gEfiDevicePathProtocolGuid, Private->GopDevicePath);\r
+\r
+FreeGopDevicePath:\r
+  if (Private->GopDevicePath != NULL) {\r
+    FreePool (Private->GopDevicePath);\r
   }\r
 \r
+RestoreAttributes:\r
+  Private->PciIo->Attributes (Private->PciIo, EfiPciIoAttributeOperationSet,\r
+                    Private->OriginalPciAttributes, NULL);\r
+\r
+ClosePciIo:\r
+  gBS->CloseProtocol (Controller, &gEfiPciIoProtocolGuid,\r
+         This->DriverBindingHandle, Controller);\r
+\r
+FreePrivate:\r
+  FreePool (Private);\r
+\r
   return Status;\r
 }\r
 \r

diff --git a/OvmfPkg/QemuVideoDxe/Gop.c b/OvmfPkg/QemuVideoDxe/Gop.c
index 30aac7f95fa89b07b692bc3734d043f1df5fa57a..1b7db329b9b3e585a6c3c8e1d66dfef436b403a7 100644 (file)
--- a/OvmfPkg/QemuVideoDxe/Gop.c
+++ b/OvmfPkg/QemuVideoDxe/Gop.c
@@ -176,7 +176,6 @@ Routine Description:
     gBS->FreePool (Private->LineBuffer);\r
   }\r
 \r
-  Private->LineBuffer = NULL;\r
   Private->LineBuffer = AllocatePool (4 * ModeData->HorizontalResolution);\r
   if (Private->LineBuffer == NULL) {\r
     return EFI_OUT_OF_RESOURCES;\r
@@ -321,13 +320,14 @@ QemuVideoGraphicsOutputConstructor (
   if (EFI_ERROR (Status)) {\r
     return Status;\r
   }\r
+\r
   Status = gBS->AllocatePool (\r
                   EfiBootServicesData,\r
                   sizeof (EFI_GRAPHICS_OUTPUT_MODE_INFORMATION),\r
                   (VOID **) &Private->GraphicsOutput.Mode->Info\r
                   );\r
   if (EFI_ERROR (Status)) {\r
-    return Status;\r
+    goto FreeMode;\r
   }\r
   Private->GraphicsOutput.Mode->MaxMode = (UINT32) Private->MaxMode;\r
   Private->GraphicsOutput.Mode->Mode    = GRAPHICS_OUTPUT_INVALIDE_MODE_NUMBER;\r
@@ -337,7 +337,11 @@ QemuVideoGraphicsOutputConstructor (
   //\r
   // Initialize the hardware\r
   //\r
-  GraphicsOutput->SetMode (GraphicsOutput, 0);\r
+  Status = GraphicsOutput->SetMode (GraphicsOutput, 0);\r
+  if (EFI_ERROR (Status)) {\r
+    goto FreeInfo;\r
+  }\r
+\r
   DrawLogo (\r
     Private,\r
     Private->ModeData[Private->GraphicsOutput.Mode->Mode].HorizontalResolution,\r
@@ -345,6 +349,15 @@ QemuVideoGraphicsOutputConstructor (
     );\r
 \r
   return EFI_SUCCESS;\r
+\r
+FreeInfo:\r
+  FreePool (Private->GraphicsOutput.Mode->Info);\r
+\r
+FreeMode:\r
+  FreePool (Private->GraphicsOutput.Mode);\r
+  Private->GraphicsOutput.Mode = NULL;\r
+\r
+  return Status;\r
 }\r
 \r
 EFI_STATUS\r
@@ -363,6 +376,10 @@ Returns:
 \r
 --*/\r
 {\r
+  if (Private->LineBuffer != NULL) {\r
+    FreePool (Private->LineBuffer);\r
+  }\r
+\r
   if (Private->GraphicsOutput.Mode != NULL) {\r
     if (Private->GraphicsOutput.Mode->Info != NULL) {\r
       gBS->FreePool (Private->GraphicsOutput.Mode->Info);\r

diff --git a/OvmfPkg/QemuVideoDxe/Initialize.c b/OvmfPkg/QemuVideoDxe/Initialize.c
index 305797bd50d20d7202595c57d27e41a2510e4687..37744786cb6645d79a8757441fe54b9033b333a7 100644 (file)
--- a/OvmfPkg/QemuVideoDxe/Initialize.c
+++ b/OvmfPkg/QemuVideoDxe/Initialize.c
@@ -176,6 +176,9 @@ QemuVideoCirrusModeSetup (
   Private->ModeData = AllocatePool (\r
                         sizeof (Private->ModeData[0]) * QEMU_VIDEO_CIRRUS_MODE_COUNT\r
                         );\r
+  if (Private->ModeData == NULL) {\r
+    return EFI_OUT_OF_RESOURCES;\r
+  }\r
   ModeData = Private->ModeData;\r
   VideoMode = &QemuVideoCirrusModes[0];\r
   for (Index = 0; Index < QEMU_VIDEO_CIRRUS_MODE_COUNT; Index ++) {\r
@@ -228,6 +231,9 @@ QemuVideoBochsModeSetup (
   Private->ModeData = AllocatePool (\r
                         sizeof (Private->ModeData[0]) * QEMU_VIDEO_BOCHS_MODE_COUNT\r
                         );\r
+  if (Private->ModeData == NULL) {\r
+    return EFI_OUT_OF_RESOURCES;\r
+  }\r
   ModeData = Private->ModeData;\r
   VideoMode = &QemuVideoBochsModes[0];\r
   for (Index = 0; Index < QEMU_VIDEO_BOCHS_MODE_COUNT; Index ++) {\r