From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BA21C21DF9668 for ; Thu, 27 Jul 2017 08:29:52 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Jul 2017 08:31:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,419,1496127600"; d="scan'208";a="292244836" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 27 Jul 2017 08:31:56 -0700 Received: from fmsmsx123.amr.corp.intel.com (10.18.125.38) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 27 Jul 2017 08:31:56 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx123.amr.corp.intel.com (10.18.125.38) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 27 Jul 2017 08:31:56 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.197]) with mapi id 14.03.0319.002; Thu, 27 Jul 2017 23:31:53 +0800 From: "Gao, Liming" To: "Marvin.Haeuser@outlook.com" , "edk2-devel@lists.01.org" CC: "Kinney, Michael D" Thread-Topic: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. Thread-Index: AQHTA5vus1udQdORhEegPjUTqWKZfaJjLSJAgAADdaCABJ2G8A== Date: Thu, 27 Jul 2017 15:31:52 +0000 Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D75CC1D@shsmsx102.ccr.corp.intel.com> References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Jul 2017 15:29:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Marvin: Once this API is exposed as the public library API, what code will you up= date to consume this API? And, for the patch, I have two comments.=20 1) Original logic uses PcdVerifyNodeInList to turn on the verification of N= odeInList. New logic always turns on it. Because this check takes much over= head, this PCD is FALSE as the default value in MdePkg.dec. 2) In IsListEmpty(), ASSERT() is missing for InternalBaseLibIsListValid (Li= stHead). Thanks Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ma= rvin H?user > Sent: Tuesday, July 25, 2017 12:53 AM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D > Subject: Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() functi= on. >=20 > Hey Mike, >=20 > In contrast to setting 'BackLink' and 'ForwardLink' to dummy values such = as EFI_BAD_POINTER, verifying that a node is part of a known > list is a safe way to determine whether a link is still valid without nee= ding to keep the memory allocated. If the Node, which had its links > set to dummys, is freed, the dummy values may be overwriten and the check= would pass despite the node being invalid. This can, for > example, be used in OpenKabylake's PchSmiDispatcher, which does the descr= ibed check against EFI_BAD_POINTER (though if I > remember correctly, the value is actually never explicitely set). The iss= ue is that some function may pass a handle that was unregistered > to the function. IsNodeInList() can be used to determine whether it was p= reviously removed. >=20 > Thanks for your response! >=20 > Regards, > Marvin. >=20 > > -----Original Message----- > > From: Kinney, Michael D [mailto:michael.d.kinney@intel.com] > > Sent: Monday, July 24, 2017 6:31 PM > > To: Marvin H=E4user ; edk2- > > devel@lists.01.org; Kinney, Michael D > > Cc: Gao, Liming > > Subject: RE: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. > > > > Hi Marvin, > > > > Can you provide a few more details on why you would like to see tis int= ernal > > function promoted to a library class API? > > > > Thanks, > > > > Mike > > > > > -----Original Message----- > > > From: Marvin H=E4user [mailto:Marvin.Haeuser@outlook.com] > > > Sent: Sunday, July 23, 2017 3:11 AM > > > To: edk2-devel@lists.01.org > > > Cc: Kinney, Michael D ; Gao, Liming > > > > > > Subject: [PATCH 1/2] MdePkg/BaseLib: Add IsNodeInList() function. > > > > > > This patch adds IsNodeInList() to BaseLib, which verifies the given > > > Node is part of the doubly-linked List provided. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Marvin Haeuser > > > --- > > > MdePkg/Library/BaseLib/LinkedList.c | 70 > > > +++++++++++++++++++- > > > MdePkg/Include/Library/BaseLib.h | 25 +++++++ > > > MdePkg/Library/BaseLib/BaseLibInternals.h | 28 -------- > > > 3 files changed, 94 insertions(+), 29 deletions(-) > > > > > > diff --git a/MdePkg/Library/BaseLib/LinkedList.c > > > b/MdePkg/Library/BaseLib/LinkedList.c > > > index ba373f4b7be3..b364ae41c647 100644 > > > --- a/MdePkg/Library/BaseLib/LinkedList.c > > > +++ b/MdePkg/Library/BaseLib/LinkedList.c > > > @@ -1,7 +1,7 @@ > > > /** @file > > > Linked List Library Functions. > > > > > > - Copyright (c) 2006 - 2013, Intel Corporation. All rights > > > reserved.
> > > + Copyright (c) 2006 - 2017, Intel Corporation. All rights > > > reserved.
> > > This program and the accompanying materials > > > are licensed and made available under the terms and conditions of > > > the BSD License > > > which accompanies this distribution. The full text of the license > > > may be found at @@ -113,6 +113,74 @@ InternalBaseLibIsNodeInList ( } > > > > > > /** > > > + Checks whether Node is part of a doubly-linked list. > > > + > > > + If List is NULL, then ASSERT(). > > > + If List->ForwardLink is NULL, then ASSERT(). > > > + If List->BackLink is NULL, then ASSERT(). > > > + If Node is NULL, then ASSERT(); > > > + If PcdMaximumLinkedListLength is not zero, and List contains > > > more than > > > + PcdMaximumLinkedListLength nodes, then ASSERT(). > > > + > > > + @param List A pointer to a node in a linked list. > > > + @param Node A pointer to the node to locate. > > > + > > > + @retval TRUE Node is in List. > > > + @retval FALSE Node isn't in List, or List is invalid. > > > + > > > +**/ > > > +BOOLEAN > > > +EFIAPI > > > +IsNodeInList ( > > > + IN CONST LIST_ENTRY *List, > > > + IN CONST LIST_ENTRY *Node > > > + ) > > > +{ > > > + UINTN Count; > > > + CONST LIST_ENTRY *Ptr; > > > + > > > + // > > > + // Test the validity of List and Node // ASSERT (List !=3D NULL)= ; > > > + ASSERT (List->ForwardLink !=3D NULL); ASSERT (List->BackLink !=3D > > > + NULL); ASSERT (Node !=3D NULL); > > > + > > > + // > > > + // ASSERT List not too long > > > + // > > > + ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE)); > > > + > > > + Count =3D 0; > > > + Ptr =3D List; > > > + > > > + // > > > + // Check to see if Node is a member of List. > > > + // Exit early if the number of nodes in List >=3D > > > PcdMaximumLinkedListLength > > > + // > > > + do { > > > + Ptr =3D Ptr->ForwardLink; > > > + if (PcdGet32 (PcdMaximumLinkedListLength) > 0) { > > > + Count++; > > > + > > > + // > > > + // Return if the linked list is too long > > > + // > > > + if (Count =3D=3D PcdGet32 (PcdMaximumLinkedListLength)) { > > > + return (BOOLEAN)(Ptr =3D=3D Node); > > > + } > > > + } > > > + > > > + if (Ptr =3D=3D Node) { > > > + return TRUE; > > > + } > > > + } while (Ptr !=3D List); > > > + > > > + return FALSE; > > > +} > > > + > > > +/** > > > Initializes the head node of a doubly-linked list, and returns the > > > pointer to > > > the head node of the doubly-linked list. > > > > > > diff --git a/MdePkg/Include/Library/BaseLib.h > > > b/MdePkg/Include/Library/BaseLib.h > > > index 791849b80406..4f3f4fd51f3f 100644 > > > --- a/MdePkg/Include/Library/BaseLib.h > > > +++ b/MdePkg/Include/Library/BaseLib.h > > > @@ -2869,6 +2869,31 @@ PathCleanUpDirectories( > > > > > > > > > /** > > > + Checks whether Node is part of a doubly-linked list. > > > + > > > + If List is NULL, then ASSERT(). > > > + If List->ForwardLink is NULL, then ASSERT(). > > > + If List->BackLink is NULL, then ASSERT(). > > > + If Node is NULL, then ASSERT(); > > > + If PcdMaximumLinkedListLength is not zero, and List contains > > > more than > > > + PcdMaximumLinkedListLength nodes, then ASSERT(). > > > + > > > + @param List A pointer to a node in a linked list. > > > + @param Node A pointer to the node to locate. > > > + > > > + @retval TRUE Node is in List. > > > + @retval FALSE Node isn't in List, or List is invalid. > > > + > > > +**/ > > > +BOOLEAN > > > +EFIAPI > > > +IsNodeInList ( > > > + IN CONST LIST_ENTRY *List, > > > + IN CONST LIST_ENTRY *Node > > > + ); > > > + > > > + > > > +/** > > > Initializes the head node of a doubly linked list, and returns the > > > pointer to > > > the head node of the doubly linked list. > > > > > > diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h > > > b/MdePkg/Library/BaseLib/BaseLibInternals.h > > > index ea387ce37d27..9dca97a0dcc9 100644 > > > --- a/MdePkg/Library/BaseLib/BaseLibInternals.h > > > +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h > > > @@ -340,34 +340,6 @@ InternalSwitchStack ( > > > > > > > > > /** > > > - Worker function that locates the Node in the List. > > > - > > > - By searching the List, finds the location of the Node in List. > > > At the same time, > > > - verifies the validity of this list. > > > - > > > - If List is NULL, then ASSERT(). > > > - If List->ForwardLink is NULL, then ASSERT(). > > > - If List->backLink is NULL, then ASSERT(). > > > - If Node is NULL, then ASSERT(); > > > - If PcdMaximumLinkedListLength is not zero, and prior to insertion > > > the number > > > - of nodes in ListHead, including the ListHead node, is greater than > > > or > > > - equal to PcdMaximumLinkedListLength, then ASSERT(). > > > - > > > - @param List A pointer to a node in a linked list. > > > - @param Node A pointer to one nod. > > > - > > > - @retval TRUE Node is in List. > > > - @retval FALSE Node isn't in List, or List is invalid. > > > - > > > -**/ > > > -BOOLEAN > > > -EFIAPI > > > -IsNodeInList ( > > > - IN CONST LIST_ENTRY *List, > > > - IN CONST LIST_ENTRY *Node > > > - ); > > > - > > > -/** > > > Worker function that returns a bit field from Operand. > > > > > > Returns the bitfield specified by the StartBit and the EndBit from > > > Operand. > > > -- > > > 2.12.2.windows.2 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel