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 provided the second previously (June and
July 2023), and literally nobody commented in edk2-rfc or edk2-devel.
Chip
-----Original Message-----
From: Laszlo Ersek
Sent: Monday, November 13, 2023 9:59 AM
To: devel@edk2.groups.io ; chip.program...@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.program...@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.program...@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 (#111425): https://edk2.groups.io/g/devel/message/111425
Mute This Topic: https://groups.io/mt/102191640/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
