Re: [PATCH v1 1/2] MdeModulePkg/NetLib.h: Fix the possible NULL pointer dereference issue.

[Laszlo Ersek <lersek@redhat.com>]

Hi Jiaxin,

On 01/15/19 02:26, Jiaxin Wu wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1456
> 
> For the NET_LIST_FOR_EACH & NET_LIST_FOR_EACH_SAFE, "Entry" should be
> checked whether it's NULL or not instead of using the pointer directly.
> 
> Cc: Wu Hao A <hao.a.wu@intel.com>
> Cc: Gao Liming <liming.gao@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  MdeModulePkg/Include/Library/NetLib.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h b/MdeModulePkg/Include/Library/NetLib.h
> index 0977973921..5b1307553a 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -616,21 +616,21 @@ NetRandomInitSeed (
>  
>  //
>  // Iterate through the double linked list. It is NOT delete safe
>  //
>  #define NET_LIST_FOR_EACH(Entry, ListHead) \
> -  for(Entry = (ListHead)->ForwardLink; Entry != (ListHead); Entry = Entry->ForwardLink)
> +  for(Entry = (ListHead)->ForwardLink; Entry != (ListHead), Entry != NULL; Entry = Entry->ForwardLink)

(1) The linked list from BaseLib (LIST_ENTRY) always has at least one
element (the head element), and the list is empty if the head element
points back to itself. In other words, ForwardLink may never be NULL.

So why is it necessary to check against that case here?

(2) If the NULL check is indeed necessary for some reason, then we
should write

  Entry != (ListHead) && Entry != NULL

in the controlling expression. Because, with the comma operator, the
(Entry != (ListHead)) expression would be evaluated, but its result
would be ignored.

Thanks,
Laszlo

>  
>  //
>  // Iterate through the double linked list. This is delete-safe.
>  // Don't touch NextEntry. Also, don't use this macro if list
>  // entries other than the Entry may be deleted when processing
>  // the current Entry.
>  //
>  #define NET_LIST_FOR_EACH_SAFE(Entry, NextEntry, ListHead) \
> -  for(Entry = (ListHead)->ForwardLink, NextEntry = Entry->ForwardLink; \
> -      Entry != (ListHead); \
> +  for(Entry = (ListHead)->ForwardLink, (Entry != NULL) ? (NextEntry = Entry->ForwardLink) : (Entry = NULL); \
> +      Entry != (ListHead), Entry != NULL; \
>        Entry = NextEntry, NextEntry = Entry->ForwardLink \
>       )
>  
>  //
>  // Make sure the list isn't empty before getting the first/last record.
>