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

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;
+    return EFI_OUT_OF_RESOURCES;
-    goto Error;
   }
 
   //
   // 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 (Private->GopDevicePath == NULL) {
-    //
+      Status = EFI_OUT_OF_RESOURCES;
-    // If RemainingDevicePath is the End of Device Path Node, 
+      goto RestoreAttributes;
-    // don't create child device and return EFI_SUCCESS
+    }
-    //
-    Private->GopDevicePath = NULL;
   }
-    
+
+  //
+  // 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) {
-    //
+    return EFI_SUCCESS;
-    // 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);
-
-    Status = gBS->InstallMultipleProtocolInterfaces (
-                    &Private->Handle,
-                    &gEfiGraphicsOutputProtocolGuid,
-                    &Private->GraphicsOutput,
-                    NULL
-                    );
-    if (EFI_ERROR (Status)) {
-      goto Error;
-    }
-
-    Status = gBS->OpenProtocol (
-                  Controller,
-                  &gEfiPciIoProtocolGuid,
-                  (VOID **) &ChildPciIo,
-                  This->DriverBindingHandle,
-                  Private->Handle,
-                  EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER
-                  );
-
-    if (EFI_ERROR (Status)) {
-      goto Error;
-    }
   }
 
-Error:
+  //
+  // Start the GOP software stack.
+  //
+  Status = QemuVideoGraphicsOutputConstructor (Private);
   if (EFI_ERROR (Status)) {
-    if (Private) {
+    goto FreeModeData;
-      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
-              );
-      }
-
-      gBS->FreePool (Private);
-    }
   }
 
+  Status = gBS->InstallMultipleProtocolInterfaces (
+                  &Private->Handle,
+                  &gEfiGraphicsOutputProtocolGuid,
+                  &Private->GraphicsOutput,
+                  NULL
+                  );
+  if (EFI_ERROR (Status)) {
+    goto DestructQemuVideoGraphics;
+  }
+
+  //
+  // 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)) {
+    goto UninstallGop;
+  }
+
+  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;
 }
 

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

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 ++) {