Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak and also detect failed memory allocation #1084

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vincent-j-zimmer
Copy link

@vincent-j-zimmer vincent-j-zimmer commented Dec 13, 2024

Description

Fixes #632

This change is responsive to #632. It fixes a memory leak and also defends against a failed memory allocation.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

Build test and boot to shell using QEMU

Integration Instructions

N/A

@github-actions github-actions bot added the impact:non-functional Does not have a functional impact label Dec 13, 2024
Copy link
Contributor

@os-d os-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have stopped using MU_CHANGE tags in mu_tiano_platforms because we have completely forked from edk2, so nothing for us to track anymore. Otherwise LGTM!

Comment on lines 67 to 69
if (GfxSiliconPolicy == NULL) {
DEBUG ((DEBUG_ERROR, "Failed to allocate Policy structure\n"));
goto Exit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Status will be uninitialized in this goto Exit path.

Suggested change
if (GfxSiliconPolicy == NULL) {
DEBUG ((DEBUG_ERROR, "Failed to allocate Policy structure\n"));
goto Exit;
if (GfxSiliconPolicy == NULL) {
DEBUG ((DEBUG_ERROR, "Failed to allocate Policy structure\n"));
Status = EFI_OUT_OF_RESOURCES;
goto Exit;

Should add EFI_OUT_OF_RESOURCES to the function return values if this is done.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Updated w/ recommendations.

if (GfxSiliconPolicy == NULL) {
DEBUG ((DEBUG_ERROR, "Failed to allocate Policy structure\n"));
goto Exit;
} // Mu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} // Mu
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in Oliver's comment, the "Mu" comments can be dropped.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Updated w/ recommendation.

// We only translate the GFX ports #0 exposed to platform from conf data
GfxSiliconPolicy[0].Power_State_Port = GfxEnablePort0;

Status = SetPolicy (&gPolicyDataGFXGuid, POLICY_ATTRIBUTE_FINALIZED, GfxSiliconPolicy, sizeof (DefaultQemuGfxPolicy));

if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_ERROR, "%a Failed to update GFX policy per configuration data - %r!!!\n", __func__, Status));
ASSERT (FALSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to your change, but I suggest updating this assert so it is more useful when printed. ASSERT (FALSE) is not very helpful.

Suggested change
ASSERT (FALSE);
ASSERT_EFI_ERROR (Status);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Updated w/ recommendations.

@makubacki
Copy link
Member

CI is failing because uncrustify needs to be run. I suggest you do that following the VS Code plugin instructions here: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Code-Formatting#recommended-usage-visual-studio-vs-code-plugin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:non-functional Does not have a functional impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ConfigDataGfx Does Not Free Memory
3 participants