Re: [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications.

["Gao, Liming" <liming.gao@intel.com>]

Marvin:
  If PcdVerifyNodeInList is set TRUE, InternalBaseLibIsListValid() will be checked twice in two ASSERT(). In the original logic, InternalBaseLibIsListValid only runs once. Could we work out the same check logic?

Thanks
Liming
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Marvin H?user
> Sent: Wednesday, August 2, 2017 9:12 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2] [PATCH v2 2/2] MdePkg/BaseLib: Update internal LinkedList verifications.
> 
> 1) Replace InternalBaseLibIsNodeInList() with
>    InternalBaseLibIsListValid().
>    - The verification whether Node is within the doubly-linked List
>      is now done by IsNodeInList().
>    - Whether the list is valid is returned.
> 
> 2) The comments within InsertHeadList() and InsertTailList() stated
>    that it is checked whether Entry is not part of the doubly-linked
>    list. This was not done as argument 3 of
>    InternalBaseLibIsNodeInList() indicated whether the check is done,
>    not whether to check if the node is or is not in the list. This
>    has been fixed by using IsNodeInList() for the ASSERTs.
> 
> V2:
>   - Fix IsListEmpty() to ASSERT when the passed list is invalid.
>   - Introduce the VERIFY_IS_NODE_IN_LIST() macro to only verify whether the
>     passed node is part of the list when PcdVerifyNodeInList is TRUE.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdePkg/Library/BaseLib/LinkedList.c | 105 +++++++++-----------
>  1 file changed, 47 insertions(+), 58 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/LinkedList.c b/MdePkg/Library/BaseLib/LinkedList.c
> index 484ee836b8c2..6864fae7d939 100644
> --- a/MdePkg/Library/BaseLib/LinkedList.c
> +++ b/MdePkg/Library/BaseLib/LinkedList.c
> @@ -15,25 +15,38 @@
>  #include "BaseLibInternals.h"
> 
>  /**
> -  Worker function that locates the Node in the List.
> +  If PcdVerifyNodeInList is TRUE, checks whether FirstNode and SecondNode are
> +  part of the same doubly-linked list.
> 
> -  By searching the List, finds the location of the Node in List. At the same time,
> -  verifies the validity of this list.
> +  If FirstEntry is NULL, then ASSERT().
> +  If FirstEntry->ForwardLink is NULL, then ASSERT().
> +  If FirstEntry->BackLink is NULL, then ASSERT().
> +  If SecondEntry is NULL, then ASSERT();
> +  If PcdMaximumLinkedListLength is not zero, and List contains more than
> +  PcdMaximumLinkedListLength nodes, then ASSERT().
> +
> +  @param  FirstEntry   A pointer to a node in a linked list.
> +  @param  SecondEntry  A pointer to the node to locate.
> +
> +  @retval TRUE   SecondEntry is in the same doubly-linked list as FirstEntry
> +                 or PcdVerifyNodeInList is FALSE.
> +  @retval FALSE  SecondEntry isn't in the same doubly-linked list as FirstEntry,
> +                 or FirstEntry is invalid.
> +
> +**/
> +#define VERIFY_IS_NODE_IN_LIST(FirstEntry, SecondEntry)  \
> +  (!FeaturePcdGet (PcdVerifyNodeInList) || IsNodeInList ((FirstEntry), (SecondEntry)))
> +
> +/**
> +  Worker function that 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 PcdVerifyNodeInList is TRUE and DoMembershipCheck is TRUE and Node
> -  is in not a member of List, then return FALSE
> +  If List->BackLink 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 a node in a linked list.
> -  @param  VerifyNodeInList  TRUE if a check should be made to see if Node is a
> -                            member of List.  FALSE if no membership test should
> -                            be performed.
> 
>    @retval   TRUE if PcdVerifyNodeInList is FALSE
>    @retval   TRUE if DoMembershipCheck is FALSE
> @@ -45,10 +58,8 @@
>  **/
>  BOOLEAN
>  EFIAPI
> -InternalBaseLibIsNodeInList (
> -  IN CONST LIST_ENTRY  *List,
> -  IN CONST LIST_ENTRY  *Node,
> -  IN BOOLEAN           VerifyNodeInList
> +InternalBaseLibIsListValid (
> +  IN CONST LIST_ENTRY  *List
>    )
>  {
>    UINTN             Count;
> @@ -60,40 +71,11 @@ InternalBaseLibIsNodeInList (
>    ASSERT (List != NULL);
>    ASSERT (List->ForwardLink != NULL);
>    ASSERT (List->BackLink != NULL);
> -  ASSERT (Node != NULL);
> -
> -  Count = 0;
> -  Ptr   = List;
> -
> -  if (FeaturePcdGet (PcdVerifyNodeInList) && VerifyNodeInList) {
> -    //
> -    // Check to see if Node is a member of List.
> -    // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength
> -    //
> -    do {
> -      Ptr = Ptr->ForwardLink;
> -      if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> -        Count++;
> -        //
> -        // ASSERT() if the linked list is too long
> -        //
> -        ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength));
> -
> -        //
> -        // Return if the linked list is too long
> -        //
> -        if (Count >= PcdGet32 (PcdMaximumLinkedListLength)) {
> -          return (BOOLEAN)(Ptr == Node);
> -        }
> -      }
> -    } while ((Ptr != List) && (Ptr != Node));
> -
> -    if (Ptr != Node) {
> -      return FALSE;
> -    }
> -  }
> 
>    if (PcdGet32 (PcdMaximumLinkedListLength) > 0) {
> +    Count = 0;
> +    Ptr   = List;
> +
>      //
>      // Count the total number of nodes in List.
>      // Exit early if the number of nodes in List >= PcdMaximumLinkedListLength
> @@ -104,9 +86,9 @@ InternalBaseLibIsNodeInList (
>      } while ((Ptr != List) && (Count < PcdGet32 (PcdMaximumLinkedListLength)));
> 
>      //
> -    // ASSERT() if the linked list is too long
> +    // return whether linked list is too long
>      //
> -    ASSERT (Count < PcdGet32 (PcdMaximumLinkedListLength));
> +    return (BOOLEAN)(Count < PcdGet32 (PcdMaximumLinkedListLength));
>    }
> 
>    return TRUE;
> @@ -244,7 +226,8 @@ InsertHeadList (
>    //
>    // ASSERT List not too long and Entry is not one of the nodes of List
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
> +  ASSERT (InternalBaseLibIsListValid (ListHead));
> +  ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry));
> 
>    Entry->ForwardLink = ListHead->ForwardLink;
>    Entry->BackLink = ListHead;
> @@ -285,7 +268,8 @@ InsertTailList (
>    //
>    // ASSERT List not too long and Entry is not one of the nodes of List
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (ListHead, Entry, FALSE));
> +  ASSERT (InternalBaseLibIsListValid (ListHead));
> +  ASSERT (!VERIFY_IS_NODE_IN_LIST (ListHead, Entry));
> 
>    Entry->ForwardLink = ListHead;
>    Entry->BackLink = ListHead->BackLink;
> @@ -323,7 +307,7 @@ GetFirstNode (
>    //
>    // ASSERT List not too long
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (List, List, FALSE));
> +  ASSERT (InternalBaseLibIsListValid (List));
> 
>    return List->ForwardLink;
>  }
> @@ -359,7 +343,8 @@ GetNextNode (
>    //
>    // ASSERT List not too long and Node is one of the nodes of List
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE));
> +  ASSERT (InternalBaseLibIsListValid (List));
> +  ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node));
> 
>    return Node->ForwardLink;
>  }
> @@ -395,7 +380,8 @@ GetPreviousNode (
>    //
>    // ASSERT List not too long and Node is one of the nodes of List
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE));
> +  ASSERT (InternalBaseLibIsListValid (List));
> +  ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node));
> 
>    return Node->BackLink;
>  }
> @@ -428,7 +414,7 @@ IsListEmpty (
>    //
>    // ASSERT List not too long
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (ListHead, ListHead, FALSE));
> +  ASSERT (InternalBaseLibIsListValid (ListHead));
> 
>    return (BOOLEAN)(ListHead->ForwardLink == ListHead);
>  }
> @@ -469,7 +455,8 @@ IsNull (
>    //
>    // ASSERT List not too long and Node is one of the nodes of List
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE));
> +  ASSERT (InternalBaseLibIsListValid (List));
> +  ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node));
> 
>    return (BOOLEAN)(Node == List);
>  }
> @@ -507,7 +494,8 @@ IsNodeAtEnd (
>    //
>    // ASSERT List not too long and Node is one of the nodes of List
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (List, Node, TRUE));
> +  ASSERT (InternalBaseLibIsListValid (List));
> +  ASSERT (VERIFY_IS_NODE_IN_LIST (List, Node));
> 
>    return (BOOLEAN)(!IsNull (List, Node) && List->BackLink == Node);
>  }
> @@ -554,7 +542,8 @@ SwapListEntries (
>    //
>    // ASSERT Entry1 and Entry2 are in the same linked list
>    //
> -  ASSERT (InternalBaseLibIsNodeInList (FirstEntry, SecondEntry, TRUE));
> +  ASSERT (InternalBaseLibIsListValid (FirstEntry));
> +  ASSERT (VERIFY_IS_NODE_IN_LIST (FirstEntry, SecondEntry));
> 
>    //
>    // Ptr is the node pointed to by FirstEntry->ForwardLink
> --
> 2.12.2.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel