Re: [edk2-devel] [PATCH v1 1/1] Bug 2861 - HiiDatabaseDxe, ConfigRouting.c, GetElementsFromRequest incorrect error handling.

["Laszlo Ersek" <lersek@redhat.com>]

Hi Charles,

On 10/26/23 03:05, Charles Hyde wrote:
> From: Charles Hyde <chip.programmer@att.net>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2861
> 
> I believe the attached ConfigRouting.txt patch will resolve bug 2861, plus
> resolve an uninitialized pointer issue in HiiConfigRoutingExportConfig().
> The uninitialized pointer was identified when running the EDK2 Self
> Certification Test with all tests selected, having caused the CPU to issue
> an exception error (most times) or completely trashed the system
> (sometimes).
> 
> I found a second instance of GetElementsFromRequest(), located in HiiLib.c,
> that also needed an update.  The attached patch should address this bug and
> more.
> 
> Signed-off-by: Charles Hyde <chip.programmer@att.net>
> ---

Thanks for analyzing and fixing these bugs.

Can you please split the separate fixes to separate patches?

Also, the patch looks garbled; it shouldn't be attached / pasted but
sent with git-send-email. Are you familiar with git-send-email?

Here's the official docs:

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

and some unofficial tips:

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

Third, I suggest not to comment out, with /* */, dead code (such as a
subcondition that always evaluates to false or true); instead, remove
it, and explain in the commit message (or, if necessary, in a code
comment) why that condition is a tautology. If the condition or argument
is nontrivial, consider using an ASSERT().

Laszlo


> 
> diff --git a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> index 63a37ab59a..c3dc7bf558 100644
> --- a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> +++ b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
> @@ -2272,8 +2272,14 @@ GetElementsFromRequest (
> {
>   EFI_STRING  TmpRequest;
> 
> +  ASSERT (ConfigRequest != NULL);
> +  if (ConfigRequest == NULL)
> +    return FALSE;
> +
>   TmpRequest = StrStr (ConfigRequest, L"PATH=");
>   ASSERT (TmpRequest != NULL);
> +  if (TmpRequest == NULL)
> +    return FALSE;
> 
>   if ((StrStr (TmpRequest, L"&OFFSET=") != NULL) || (StrStr (TmpRequest,
> L"&") != NULL)) {
>     return TRUE;
> diff --git a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> index 5ae6189a28..0b39f156f3 100644
> --- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> +++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
> @@ -420,14 +420,19 @@ AppendToMultiString (
>   }
> 
>   AppendStringSize = StrSize (AppendString);
> +  if (AppendStringSize <= sizeof(*AppendString))    // If the string is
> empty, there is no need to proceed further.
> +    return EFI_SUCCESS;
> +
>   MultiStringSize  = StrSize (*MultiString);
>   MaxLen           = MAX_STRING_LENGTH / sizeof (CHAR16);
> 
>   //
>   // Enlarge the buffer each time when length exceeds MAX_STRING_LENGTH.
>   //
> -  if ((MultiStringSize + AppendStringSize > MAX_STRING_LENGTH) ||
> -      (MultiStringSize > MAX_STRING_LENGTH))
> +  if ((MultiStringSize + AppendStringSize > MAX_STRING_LENGTH) /*||
> +      (MultiStringSize > MAX_STRING_LENGTH)*/)  // There is no need to
> check the second part.
> +                                                // If the first part is
> false, the second part will always be false.
> +                                                // If the second part is
> true, the first part must also be true.
>   {
>     *MultiString = (EFI_STRING)ReallocatePool (
>                                  MultiStringSize,
> @@ -1800,8 +1805,14 @@ GetElementsFromRequest (
> {
>   EFI_STRING  TmpRequest;
> 
> +  ASSERT (ConfigRequest != NULL);
> +  if (ConfigRequest == NULL)
> +    return FALSE;
> +
>   TmpRequest = StrStr (ConfigRequest, L"PATH=");
>   ASSERT (TmpRequest != NULL);
> +  if (TmpRequest == NULL)
> +    return FALSE;
> 
>   if ((StrStr (TmpRequest, L"&OFFSET=") != NULL) || (StrStr (TmpRequest,
> L"&") != NULL)) {
>     return TRUE;
> @@ -5292,6 +5303,7 @@ HiiConfigRoutingExportConfig (
>     //
>     IfrDataParsedFlag = FALSE;
>     Progress          = NULL;
> +    AccessResults     = NULL;
>     HiiHandle         = NULL;
>     DefaultResults    = NULL;
>     Database          = NULL;
> @@ -5326,6 +5338,14 @@ HiiConfigRoutingExportConfig (
>                              &AccessResults
>                              );
>     if (EFI_ERROR (Status)) {
> +
> +      // If an error was returned, then do not believe any results in
> these
> two pointers.
> +      Progress = NULL;
> +      if (AccessResults) {
> +        FreePool (AccessResults);
> +        AccessResults = NULL;
> +      }
> +
>       //
>       // Update AccessResults by getting default setting from IFR when
> HiiPackage is registered to HiiHandle
>       //
> @@ -5350,6 +5370,17 @@ HiiConfigRoutingExportConfig (
>     }
> 
>     if (!EFI_ERROR (Status)) {
> +
> +      // If AccessResults == NULL, there is nothing to be done.
> +      if (AccessResults == NULL) {
> +        Progress = NULL;
> +
> +        if (ConfigRequest != NULL)
> +          FreePool (ConfigRequest);
> +
> +        continue;
> +      }
> +
>       //
>       // Update AccessResults by getting default setting from IFR when
> HiiPackage is registered to HiiHandle
>       //
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111158): https://edk2.groups.io/g/devel/message/111158
Mute This Topic: https://groups.io/mt/102191640/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-