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

ArmPkg: MdeModulePkg: OvmfPkg: SecurityPkg: ShellPkg: fix [-Werror=maybe-uninitialized] #10603

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ghbaccount
Copy link
Contributor

Description

Sometimes I compile projects using old gcc (gcc-8.1.0)
This time I've tried to build ArmVirtPkg/ArmVirtQemu.dsc and OvmfPkg/OvmfPkgIa{32,64}.dsc
and found a bunch of similar warnings preventing from complete project build.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

The changes look trivial.

Integration Instructions

N/A

Dispatcher.c:1639:34: error: 'FvMigrationFlags' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]
(((FvMigrationFlags & FLAGS_FV_MIGRATE_BEFORE_PEI_CORE_REENTRY) == 0) ||
                ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
Handle.c:1302:24: error: 'Prot' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]

       *Interface = Prot->Interface;
                    ~~~~^~~~~~~~~~~
cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
NvmExpressMediaSanitize.c: In function 'NvmExpressMediaClear':
NvmExpressMediaSanitize.c:337:29: error: 'Status' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]

   EFI_STATUS                Status;
                             ^~~~~~
cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
ArmTrng.c: In function 'GenerateEntropy':
ArmTrng.c:40:15: error: 'Status' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]

   EFI_STATUS  Status;
               ^~~~~~
cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
GenericQemuLoadImageLib.c: In function 'QemuLoadKernelImage':
GenericQemuLoadImageLib.c:323:5: error: 'CommandLine' may be used
  uninitialized in this function [-Werror=maybe-uninitialized]
     UnicodeSPrintAsciiFormat (
     ^~~~~~~~~~~~~~~~~~~~~~~~~~
       KernelLoadedImage->LoadOptions,
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       KernelLoadedImage->LoadOptionsSize,
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       "%a%a",
       ~~~~~~~
       (CommandLineSize == 0) ?  "" : CommandLine,
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       (InitrdSize == 0)      ?  "" : " initrd=initrd"
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       );
       ~
cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
Dmem.c: In function 'GetImageExecutionInfo':
Dmem.c:241:10: error: 'Status' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]
   return Status;
          ^~~~~~
Dmem.c:284:35: note: 'Status' was declared here
   EFI_STATUS                      Status;
                                   ^~~~~~
Dmem.c: In function 'ShellCommandRunDmem':
Dmem.c:586:27: error: 'ConformanceProfileTableAddress' may be used
  uninitialized in this function [-Werror=maybe-uninitialized]

ShellStatus = DisplayConformanceProfiles (ConformanceProfileTableAddress);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dmem.c:582:27: error: 'ImageExecutionTableAddress' may be used
  uninitialized in this function [-Werror=maybe-uninitialized]

ShellStatus = DisplayImageExecutionEntries (ImageExecutionTableAddress);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dmem.c:578:27: error: 'RtPropertiesTableAddress' may be used
  uninitialized in this function [-Werror=maybe-uninitialized]

ShellStatus = DisplayRtProperties (RtPropertiesTableAddress);
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
ArmGicLib.c: In function 'ArmGicAcknowledgeInterrupt':
ArmGicLib.c:200:18: error: 'IntId' may be used uninitialized
  in this function [-Werror=maybe-uninitialized]
     *InterruptId = IntId;
     ~~~~~~~~~~~~~^~~~~~~

cc1: all warnings being treated as errors

Signed-off-by: Mike Maslenkin <[email protected]>
@ghbaccount ghbaccount force-pushed the uninitialized_warnings_branch branch from 1add77f to 5863cad Compare January 10, 2025 00:30
@ghbaccount ghbaccount changed the title Uninitialized warnings branch ArmPkg: MdeModulePkg: OvmfPkg: SecurityPkg: ShellPkg: fix [-Werror=maybe-uninitialized] Jan 10, 2025
@ghbaccount ghbaccount marked this pull request as ready for review January 10, 2025 01:22
Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

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

Thank you for this fix.
The patch "ArmPkg: ArmGic: fix warning about uninitialized variable" looks good to me.

@@ -46,6 +46,7 @@ GenerateEntropy (

ZeroMem (Entropy, Length);

Status = EFI_INVALID_PARAMETER;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it may be better to validate the input arguments before line 47.

if ((Length ==0) || (Entropy == NULL)) {
return EFI_INVALID_PARAMETER;
}

That way it will also fix the issue at line 47 if Entropy is NULL.

Copy link
Contributor

@samimujawar samimujawar left a comment

Choose a reason for hiding this comment

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

For patch "SecurityPkg/RngDxe: fix warning about uninitialized variable", it would be better to validate the function input parameters.

Copy link
Contributor

@pierregondois pierregondois left a comment

Choose a reason for hiding this comment

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

The ShellPkg patch lgtm

@@ -192,6 +192,7 @@ ArmGicAcknowledgeInterrupt (
// Report Spurious interrupt which is what the above controllers would
// return if no interrupt was available
Value = 1023;
IntId = Value;
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I had a look at this function. Hated it. Since it does two things, and it's just weird. Wrong level of abstraction.

So I went and had a look at who's actually using the insane interface. Turns out no one is. Last user disappeared with 96c8e75 "ArmPlatformPkg/PrePeiCore: Drop MPCore variant".

So could we instead delete the function, and its prototype in ArmPkg/Include/Library/ArmGicLib.h?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants