From: "Charles Hyde" <chip.programmer@att.net>
To: <lersek@redhat.com>, <devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH v1 1/1] Bug 2861 - HiiDatabaseDxe, ConfigRouting.c, GetElementsFromRequest incorrect error handling.
Date: Sat, 18 Nov 2023 08:06:01 -0500 [thread overview]
Message-ID: <B2194B224A7B47B4B73A2EE0C677A41E@DESKTOPQUG2G9K> (raw)
In-Reply-To: <cc5c75bd-c2e7-10ff-7bc4-c52618036ac9@redhat.com>
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.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 (#111425): https://edk2.groups.io/g/devel/message/111425
Mute This Topic: https://groups.io/mt/102191640/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-11-18 13:06 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <62690423D2A24D1DBB82CD22AE44EADE.ref@DESKTOPQUG2G9K>
2023-10-26 1:05 ` [edk2-devel] [PATCH v1 1/1] Bug 2861 - HiiDatabaseDxe, ConfigRouting.c, GetElementsFromRequest incorrect error handling Charles Hyde
2023-11-13 14:59 ` Laszlo Ersek
2023-11-18 13:06 ` Charles Hyde [this message]
2023-11-21 14:55 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=B2194B224A7B47B4B73A2EE0C677A41E@DESKTOPQUG2G9K \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox