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

["Charles Hyde" <chip.programmer@att.net>]

From: Charles Hyde <chip.programmer@att.net>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2861

I believe the below 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 below patch should address
this bug and more.

This patch shown below is being sent with an alternate email program so
that all lines should be exact, without line wrapping.

The last line of this patch addresses a spelling error.  I have noticed
a lot of file updates recently where the only changes were for spelling,
so I chose to include one here in the same file I have been looking at.

I am not explaining these edits to a five year old, none of my grandkids
understand anything of what I say with regards to software engineering.

Signed-off-by: Charles Hyde <chip.programmer@att.net>
---

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..e04f56bc56 100644
--- a/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
+++ b/MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c
@@ -419,6 +419,9 @@ AppendToMultiString (
     return EFI_INVALID_PARAMETER;
   }
 
+  if (0 == *AppendString)   // We are done here if string is empty.
+    return EFI_SUCCESS;
+
   AppendStringSize = StrSize (AppendString);
   MultiStringSize  = StrSize (*MultiString);
   MaxLen           = MAX_STRING_LENGTH / sizeof (CHAR16);
@@ -426,8 +429,9 @@ AppendToMultiString (
   //
   // 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)
+    // AppendStringSize is unsigned, it cannot be less than zero.
+    // Remove unneeded check for MultiStringSize > MAX_STRING_LENGTH
   {
     *MultiString = (EFI_STRING)ReallocatePool (
                                  MultiStringSize,
@@ -1800,8 +1804,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 +5302,7 @@ HiiConfigRoutingExportConfig (
     //
     IfrDataParsedFlag = FALSE;
     Progress          = NULL;
+    AccessResults     = NULL;
     HiiHandle         = NULL;
     DefaultResults    = NULL;
     Database          = NULL;
@@ -5326,6 +5337,14 @@ HiiConfigRoutingExportConfig (
                              &AccessResults
                              );
     if (EFI_ERROR (Status)) {
+
+      // If an error was returned, then these pointers are no good.
+      Progress = NULL;
+      if (AccessResults) {
+        FreePool (AccessResults);
+        AccessResults = NULL;
+      }
+
       //
       // Update AccessResults by getting default setting from IFR when HiiPackage is registered to HiiHandle
       //
@@ -5350,6 +5369,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
       //
@@ -5370,7 +5400,7 @@ HiiConfigRoutingExportConfig (
       }
 
       //
-      // Merge the default sting from IFR code into the got setting from driver.
+      // Merge the default string from IFR code into the got setting from driver.
       //
       if (DefaultResults != NULL) {
         Status = MergeDefaultString (&AccessResults, DefaultResults);



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