From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id D5001740035 for ; Tue, 21 Nov 2023 14:55:28 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=qM/NU86LQXOI3sd0M6/4MxEPBZR0BL+ZJIkbTk6WCvQ=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1700578527; v=1; b=ZMsSkOeXu+GhyWWrU2mluskHdhbO7VmR9j6Se+l4z2WfKEbjezARQNHcdSQkZm6pORxlxVy3 FcTPlU+HY5rEgrV5MSgtO0/oCrcKy7dkWnlfLAerz8jghrnG2MkpgHV8ZOJqaUDZAwbYFXYOKDY lbhvHhQgbmZEjBetdBushWpM= X-Received: by 127.0.0.2 with SMTP id WuzmYY7687511xuKcq7tRiex; Tue, 21 Nov 2023 06:55:27 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web11.42909.1700578526852125218 for ; Tue, 21 Nov 2023 06:55:27 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-223-cDaySm0dNWaqzpyQl_PSow-1; Tue, 21 Nov 2023 09:55:22 -0500 X-MC-Unique: cDaySm0dNWaqzpyQl_PSow-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9402A3C0F456; Tue, 21 Nov 2023 14:55:22 +0000 (UTC) X-Received: from [10.39.194.228] (unknown [10.39.194.228]) by smtp.corp.redhat.com (Postfix) with ESMTPS id F00D110F44; Tue, 21 Nov 2023 14:55:21 +0000 (UTC) Message-ID: <0dc13bc0-6278-7ae2-150d-5aad1bc31f78@redhat.com> Date: Tue, 21 Nov 2023 15:55:20 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 1/1] Bug 2861 - HiiDatabaseDxe, ConfigRouting.c, GetElementsFromRequest incorrect error handling. To: Chip , devel@edk2.groups.io References: <62690423D2A24D1DBB82CD22AE44EADE.ref@DESKTOPQUG2G9K> <62690423D2A24D1DBB82CD22AE44EADE@DESKTOPQUG2G9K> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 2Dj5ZiJwfRXk2oBM5FSb8vB7x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ZMsSkOeX; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 >> >> 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 >> --- > > 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] -=-=-=-=-=-=-=-=-=-=-=-