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

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

On 11/18/23 14:06, Chip wrote:
> How many different patches are you looking for?
> 
> One patch to fix bug 2861 specifically, and a separate patch that fixes
> the uninitialized pointer issue?

I can't easily say, because I'm not familiar with HiiDatabaseDxe. So
looking at the present patch, I can only say it seems to be doing "too
much".

I generally prefer patches that do the possible *minimum* semantically.

Isolating the fix for the uninited pointer to one patch sounds good.

I'm not sure if the rest of the code code changes (i.e., the fix(es) for
bug 2861) belong to just *one* other patch though. If you think any one
of those changes makes no sense without the other changes, or else if
you think these changes are nearly identical all over, then keeping them
in one patch may be good.

Basically treat any patch (including commit message and code changes)
like an "explain like I'm five" lesson to a reviewer. Advance in small
steps, and explain liberally.

IMO it's not possible to write a patch that is "too didactic", only a
patch that's too terse.

> I provided the second previously (June
> and July 2023), and literally nobody commented in edk2-rfc or edk2-devel.

That's too bad, my apologies. The project has been facing challenges
with timely reviews.

Laszlo


> 
> Chip
> 
> 
> -----Original Message----- From: Laszlo Ersek
> Sent: Monday, November 13, 2023 9:59 AM
> To: devel@edk2.groups.io ; chip.programmer@att.net
> Subject: Re: [edk2-devel] [PATCH v1 1/1] Bug 2861 - HiiDatabaseDxe,
> ConfigRouting.c, GetElementsFromRequest incorrect error handling.
> 
> 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 (#111552): https://edk2.groups.io/g/devel/message/111552
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]
-=-=-=-=-=-=-=-=-=-=-=-