BaseTools:Expression.py Size used before Init

[Flickdm]

Description

The following example fails to be parsed correctly due to Size being used in the outer scope but initialized in the inner scope.
Size was intended to be an accumulator rather than reset on every loop.

Additionally, Zero byte PCDs are disallowed via exception due to causing AutoGen to generate illegal zero length arrays.

gPlatformPkgTokenSpaceGuid.PcdSecureBootDbxBinaryFile|{}

Problematic code:

for Item in NewPcdValueList:
      Size = 0
      # ....

if Size > 0:
      PcdValue = '{' + ', '.join(AllPcdValueList) + '}'

• Breaking change?
  • Breaking change
• Impacts security?
  • Security - Does this PR have a direct security impact?
• Includes tests?
  • Tests - Does this PR include any explicit test code?

How This Was Tested

Locally built and ran platforms. Found this when setting a 0 byte size PCD.

Integration Instructions

N/A

[makubacki]

Thank you for the review Mike. Would any Basetools/ maintainers like to add a review before this is pushed? @bexcran, @lgao4, @gapalomi, @BobCF, @YuweiChen1110

[gapalomi]

I made a quick test compiling the EmulatorPkg.
I changed line 241

-  gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
+  gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{}

with this change, the script doesnt show the "initialized" error but AutoGen.c generation its as this:

GLOBAL_REMOVE_IF_UNREFERENCED const UINT8 _gPcd_FixedAtBuild_PcdBootManagerMenuFile[1] = {};
extern const UINT8 _gPcd_FixedAtBuild_PcdBootManagerMenuFile[1];

Showing a build error:

D:\local\edk2\Build\EmulatorX64\DEBUG_VS2022\X64\MdeModulePkg\Application\UiApp\UiApp\DEBUG\AutoGen.c(340): error C2059: syntax error: '}'
D:\local\edk2\Build\EmulatorX64\DEBUG_VS2022\X64\MdeModulePkg\Universal\Console\ConPlatformDxe\ConPlatformDxe\DEBUG\AutoGen.c(315): error C2059: syntax error: '}'

@Flickdm how does your AutoGen.c shows the PCD?

[Flickdm]

@gapalomi You are correct - I ran into a similar issue and I'm not sure at this point that this change (in this form) should be merged in. Autogen is not built for a array less than 1. I would suspect a better solution would be to ASSERT in python if the value is ever less than 1 and make gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{} invalid.

Would you agree @gapalomi ?

[gapalomi]

Yes, Graceful error message its better than the current exception. I couldn't find if the spec defines the expected of an empty array, I think it should generate a pointer to null add consumer of that PCD should validate the pointer of the array before makes use of it.

[Flickdm]

So I agree with you it would be nice if it generates a pointer to NULL and sets the size to 0.

I wanted to use that to signal that it shouldn't set the DBX and just leave it undefined. I did figure out an alternative way to do this. However it's not as graceful.

#10937

Which option do you think is the more correct path? @mdkinney maybe you have thoughts?

1. Update spec to clearly define {} as invalid and add an assert to signal that this is not allowed. Thus no changes to AutoGen.
2. Update spec to clearly define {} as valid and define behavior. Thus changes to Autogen

[mdkinney]

Since the current behavior generates an exception during build, I would prefer the (1) and generate a good error message and return error from the build. I do not think it is valid for a structure PCD to have a size of 0. That way, code that gets a PCD value of a PCD that is type VOID* or a STRUCT * will always return a valid pointer and a size > 0.

[Flickdm]

Updated with exception to disallow zero byte PCDs

Impact is as follows:

INFO - Platform.dsc(1): error 3000: PCD with value '{}' cannot be used. Please provide a valid value of at least one byte.
INFO - 	PCD [gPlatformPkgTokenSpaceGuid.PcdSecureBootDbxBinaryFile] Value "{}"

[gapalomi]

Thanks @Flickdm, code looks good to me as it is.

I noticed that other improvement can be done in this piece of code.
The only condition we are checking its:
BaseTools\Source\Python\Common\Expression.py:920

                       if PcdValue.strip().startswith('{'):

This allow compile with this kind of expressions

  # Change PcdBootManagerMenuFile to UiApp
  gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{0x35, 0x44

where { its unbalanced

Should we handle this improvement in this PR or in a separate issue?

[Flickdm]

a separate issue?

Easy enough to fix. I went ahead and did so!