From d89186bc869bfedd9f83f8e52f995dfd04691521 Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Mon, 3 Mar 2014 08:40:19 +0000 Subject: 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 Reviewed-by: Jordan Justen git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@15282 6f19259b-4bc3-4df7-8a09-765794883524 --- OvmfPkg/QemuVideoDxe/Driver.c | 195 ++++++++++++++++++++------------------ OvmfPkg/QemuVideoDxe/Gop.c | 23 ++++- OvmfPkg/QemuVideoDxe/Initialize.c | 6 ++ 3 files changed, 129 insertions(+), 95 deletions(-) (limited to 'OvmfPkg') 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 ++) { -- cgit v1.2.3