[Patch 0/3] Fix a set of issues for DxeNetLib

[Patch 0/3] Fix a set of issues for DxeNetLib

[fanwang2]

From: Wang Fan <fan.wang@intel.com>

See each patch file for details.

Wang Fan (3):
  MdeModulePkg/DxeNetLib: Fix a potential issue in macro definition.
  MdeModulePkg/DxeNetLib: Add parameter check and ASSERT handling.
  MdeModulePkg/DxeNetLib: Fix an error in packet length counting.

 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 124 +++++++++++++++++++++++++----
 1 file changed, 107 insertions(+), 17 deletions(-)

-- 
1.9.5.msysgit.1

[Patch 1/3] MdeModulePkg/DxeNetLib: Fix a potential issue in macro definition.

[fanwang2]

From: Wang Fan <fan.wang@intel.com>

The NIC_ITEM_CONFIG_SIZE macro in DxeNetLib is defined as:
sizeof (NIC_IP4_CONFIG_INFO) + sizeof (EFI_IP4_ROUTE_TABLE) *
MAX_IP4_CONFIG_IN_VARIABLE. This macro should be surrounded
with parenthesis to avoid being incorrectly used.

Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan <fan.wang@intel.com>
---
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
index 26a80a7..0f00f79 100644
--- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
+++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
@@ -36,11 +36,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/MemoryAllocationLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/PrintLib.h>
 #include <Library/UefiLib.h>
 
-#define NIC_ITEM_CONFIG_SIZE   sizeof (NIC_IP4_CONFIG_INFO) + sizeof (EFI_IP4_ROUTE_TABLE) * MAX_IP4_CONFIG_IN_VARIABLE
+#define NIC_ITEM_CONFIG_SIZE   (sizeof (NIC_IP4_CONFIG_INFO) + sizeof (EFI_IP4_ROUTE_TABLE) * MAX_IP4_CONFIG_IN_VARIABLE)
 #define DEFAULT_ZERO_START     ((UINTN) ~0)
 
 //
 // All the supported IP4 maskes in host byte order.
 //
-- 
1.9.5.msysgit.1

Re: [Patch 1/3] MdeModulePkg/DxeNetLib: Fix a potential issue in macro definition.

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>


> -----Original Message-----
> From: Wang, Fan
> Sent: Wednesday, January 3, 2018 10:32 AM
> To: edk2-devel@lists.01.org
> Cc: Wang, Fan <fan.wang@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ye,
> Ting <ting.ye@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 1/3] MdeModulePkg/DxeNetLib: Fix a potential issue in
> macro definition.
> 
> From: Wang Fan <fan.wang@intel.com>
> 
> The NIC_ITEM_CONFIG_SIZE macro in DxeNetLib is defined as:
> sizeof (NIC_IP4_CONFIG_INFO) + sizeof (EFI_IP4_ROUTE_TABLE) *
> MAX_IP4_CONFIG_IN_VARIABLE. This macro should be surrounded
> with parenthesis to avoid being incorrectly used.
> 
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index 26a80a7..0f00f79 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -36,11 +36,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/DevicePathLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/UefiLib.h>
> 
> -#define NIC_ITEM_CONFIG_SIZE   sizeof (NIC_IP4_CONFIG_INFO) + sizeof
> (EFI_IP4_ROUTE_TABLE) * MAX_IP4_CONFIG_IN_VARIABLE
> +#define NIC_ITEM_CONFIG_SIZE   (sizeof (NIC_IP4_CONFIG_INFO) + sizeof
> (EFI_IP4_ROUTE_TABLE) * MAX_IP4_CONFIG_IN_VARIABLE)
>  #define DEFAULT_ZERO_START     ((UINTN) ~0)
> 
>  //
>  // All the supported IP4 maskes in host byte order.
>  //
> --
> 1.9.5.msysgit.1

[Patch 2/3] MdeModulePkg/DxeNetLib: Add parameter check and ASSERT handling.

[fanwang2]

From: Wang Fan <fan.wang@intel.com>

* Library API should check the input parameters before use, or
  ASSERT to tell it has to meet some requirements. But in DxeNetLib,
  not all functions follows this rule.
* ASSERT shouldn't be used as error handling, add some handling code
  for errors.
* Add some ASSERT commence in function notes.

Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan <fan.wang@intel.com>
---
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 119 +++++++++++++++++++++++++----
 1 file changed, 105 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
index 0f00f79..90f17b7 100644
--- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
+++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
@@ -1,9 +1,9 @@
 /** @file
   Network library.
 
-Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
 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
 http://opensource.org/licenses/bsd-license.php
@@ -196,10 +196,11 @@ SyslogLocateSnp (
   Transmit a syslog packet synchronously through SNP. The Packet
   already has the ethernet header prepended. This function should
   fill in the source MAC because it will try to locate a SNP each
   time it is called to avoid the problem if SNP is unloaded.
   This code snip is copied from MNP.
+  If Packet is NULL, then ASSERT().
 
   @param[in] Packet          The Syslog packet
   @param[in] Length          The length of the packet
 
   @retval EFI_DEVICE_ERROR   Failed to locate a usable SNP protocol
@@ -217,10 +218,12 @@ SyslogSendPacket (
   ETHER_HEAD                  *Ether;
   EFI_STATUS                  Status;
   EFI_EVENT                   TimeoutEvent;
   UINT8                       *TxBuf;
 
+  ASSERT (Packet != NULL);
+
   Snp = SyslogLocateSnp ();
 
   if (Snp == NULL) {
     return EFI_DEVICE_ERROR;
   }
@@ -308,11 +311,11 @@ ON_EXIT:
   @param[in]  Line      The line of code in the File that contains the current log
   @param[in]  Message   The log message
   @param[in]  BufLen    The lenght of the Buf
   @param[out] Buf       The buffer to put the packet data
 
-  @return The length of the syslog packet built.
+  @return The length of the syslog packet built, 0 represents no packet is built.
 
 **/
 UINT32
 SyslogBuildPacket (
   IN  UINT32                Level,
@@ -322,10 +325,11 @@ SyslogBuildPacket (
   IN  UINT8                 *Message,
   IN  UINT32                BufLen,
   OUT CHAR8                 *Buf
   )
 {
+  EFI_STATUS                Status;
   ETHER_HEAD                *Ether;
   IP4_HEAD                  *Ip4;
   EFI_UDP_HEADER            *Udp4;
   EFI_TIME                  Time;
   UINT32                    Pri;
@@ -377,12 +381,14 @@ SyslogBuildPacket (
 
   //
   // Build the syslog message body with <PRI> Timestamp  machine module Message
   //
   Pri = ((NET_SYSLOG_FACILITY & 31) << 3) | (Level & 7);
-  gRT->GetTime (&Time, NULL);
-  ASSERT ((Time.Month <= 12) && (Time.Month >= 1));
+  Status = gRT->GetTime (&Time, NULL);
+  if (EFI_ERROR (Status)) {
+    return 0;
+  }
 
   //
   // Use %a to format the ASCII strings, %s to format UNICODE strings
   //
   Len  = 0;
@@ -437,10 +443,12 @@ SyslogBuildPacket (
            __FILE__,
            __LINE__,
            NetDebugASPrint ("State transit to %a\n", Name)
          )
 
+  If Format is NULL, then ASSERT().
+
   @param Format  The ASCII format string.
   @param ...     The variable length parameter whose format is determined
                  by the Format string.
 
   @return        The buffer containing the formatted message,
@@ -455,10 +463,12 @@ NetDebugASPrint (
   )
 {
   VA_LIST                   Marker;
   CHAR8                     *Buf;
 
+  ASSERT (Format != NULL);
+
   Buf = (CHAR8 *) AllocatePool (NET_DEBUG_MSG_LEN);
 
   if (Buf == NULL) {
     return NULL;
   }
@@ -481,11 +491,12 @@ NetDebugASPrint (
   @param File     The file that contains the log.
   @param Line     The exact line that contains the log.
   @param Message  The user message to log.
 
   @retval EFI_INVALID_PARAMETER Any input parameter is invalid.
-  @retval EFI_OUT_OF_RESOURCES  Failed to allocate memory for the packet
+  @retval EFI_OUT_OF_RESOURCES  Failed to allocate memory for the packet.
+  @retval EFI_DEVICE_ERROR      Device error occurs.
   @retval EFI_SUCCESS           The log is discard because that it is more verbose
                                 than the mNetDebugLevelMax. Or, it has been sent out.
 **/
 EFI_STATUS
 EFIAPI
@@ -502,11 +513,11 @@ NetDebugOutput (
   EFI_STATUS                   Status;
 
   //
   // Check whether the message should be sent out
   //
-  if (Message == NULL) {
+  if (Message == NULL || File == NULL || Module == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   if (Level > mNetDebugLevelMax) {
     Status = EFI_SUCCESS;
@@ -535,13 +546,17 @@ NetDebugOutput (
           Line,
           Message,
           NET_SYSLOG_PACKET_LEN,
           Packet
           );
+  if (Len == 0) {
+    Status = EFI_DEVICE_ERROR;
+  } else {
+    mSyslogPacketSeq++;
+    Status = SyslogSendPacket (Packet, Len);
+  }
 
-  mSyslogPacketSeq++;
-  Status = SyslogSendPacket (Packet, Len);
   FreePool (Packet);
 
 ON_EXIT:
   FreePool (Message);
   return Status;
@@ -673,10 +688,12 @@ NetIp4IsUnicast (
 }
 
 /**
   Check whether the incoming IPv6 address is a valid unicast address.
 
+  ASSERT if Ip6 is NULL.
+
   If the address is a multicast address has binary 0xFF at the start, it is not
   a valid unicast address. If the address is unspecified ::, it is not a valid
   unicast address to be assigned to any node. If the address is loopback address
   ::1, it is also not a valid unicast address to be assigned to any physical
   interface.
@@ -693,10 +710,12 @@ NetIp6IsValidUnicast (
   )
 {
   UINT8 Byte;
   UINT8 Index;
 
+  ASSERT (Ip6 != NULL);
+
   if (Ip6->Addr[0] == 0xFF) {
     return FALSE;
   }
 
   for (Index = 0; Index < 15; Index++) {
@@ -715,10 +734,12 @@ NetIp6IsValidUnicast (
 }
 
 /**
   Check whether the incoming Ipv6 address is the unspecified address or not.
 
+  ASSERT if Ip6 is NULL.
+
   @param[in] Ip6   - Ip6 address, in network order.
 
   @retval TRUE     - Yes, unspecified
   @retval FALSE    - No
 
@@ -729,10 +750,12 @@ NetIp6IsUnspecifiedAddr (
   IN EFI_IPv6_ADDRESS       *Ip6
   )
 {
   UINT8 Index;
 
+  ASSERT (Ip6 != NULL);
+
   for (Index = 0; Index < 16; Index++) {
     if (Ip6->Addr[Index] != 0) {
       return FALSE;
     }
   }
@@ -741,10 +764,12 @@ NetIp6IsUnspecifiedAddr (
 }
 
 /**
   Check whether the incoming Ipv6 address is a link-local address.
 
+  ASSERT if Ip6 is NULL.
+
   @param[in] Ip6   - Ip6 address, in network order.
 
   @retval TRUE  - Yes, link-local address
   @retval FALSE - No
 
@@ -777,10 +802,13 @@ NetIp6IsLinkLocalAddr (
 }
 
 /**
   Check whether the Ipv6 address1 and address2 are on the connected network.
 
+  ASSERT if Ip1 or Ip2 is NULL.
+  ASSERT if PrefixLength exceeds IP6_PREFIX_MAX.
+
   @param[in] Ip1          - Ip6 address1, in network order.
   @param[in] Ip2          - Ip6 address2, in network order.
   @param[in] PrefixLength - The prefix length of the checking net.
 
   @retval TRUE            - Yes, connected.
@@ -813,11 +841,10 @@ NetIp6IsNetEqual (
   }
 
   if (Bit > 0) {
     Mask = (UINT8) (0xFF << (8 - Bit));
 
-    ASSERT (Byte < 16);
     if ((Ip1->Addr[Byte] & Mask) != (Ip2->Addr[Byte] & Mask)) {
       return FALSE;
     }
   }
 
@@ -826,10 +853,12 @@ NetIp6IsNetEqual (
 
 
 /**
   Switches the endianess of an IPv6 address
 
+  ASSERT if Ip6 is NULL.
+
   This function swaps the bytes in a 128-bit IPv6 address to switch the value
   from little endian to big endian or vice versa. The byte swapped value is
   returned.
 
   @param  Ip6 Points to an IPv6 address
@@ -844,10 +873,12 @@ Ip6Swap128 (
   )
 {
   UINT64 High;
   UINT64 Low;
 
+  ASSERT (Ip6 != NULL);
+
   CopyMem (&High, Ip6, sizeof (UINT64));
   CopyMem (&Low, &Ip6->Addr[8], sizeof (UINT64));
 
   High = SwapBytes64 (High);
   Low  = SwapBytes64 (Low);
@@ -891,10 +922,12 @@ NetRandomInitSeed (
 
 
 /**
   Extract a UINT32 from a byte stream.
 
+  ASSERT if Buf is NULL.
+
   Copy a UINT32 from a byte stream, then converts it from Network
   byte order to host byte order. Use this function to avoid alignment error.
 
   @param[in]  Buf                 The buffer to extract the UINT32.
 
@@ -907,18 +940,22 @@ NetGetUint32 (
   IN UINT8                  *Buf
   )
 {
   UINT32                    Value;
 
+  ASSERT (Buf != NULL);
+
   CopyMem (&Value, Buf, sizeof (UINT32));
   return NTOHL (Value);
 }
 
 
 /**
   Put a UINT32 to the byte stream in network byte order.
 
+  ASSERT if Buf is NULL.
+
   Converts a UINT32 from host byte order to network byte order. Then copy it to the
   byte stream.
 
   @param[in, out]  Buf          The buffer to put the UINT32.
   @param[in]       Data         The data to be converted and put into the byte stream.
@@ -929,10 +966,12 @@ EFIAPI
 NetPutUint32 (
   IN OUT UINT8                 *Buf,
   IN     UINT32                Data
   )
 {
+  ASSERT (Buf != NULL);
+
   Data = HTONL (Data);
   CopyMem (Buf, &Data, sizeof (UINT32));
 }
 
 
@@ -1027,10 +1066,12 @@ NetListRemoveTail (
 
 
 /**
   Insert a new node entry after a designated node entry of a doubly linked list.
 
+  ASSERT if PrevEntry or NewEntry is NULL.
+
   Inserts a new node entry donated by NewEntry after the node entry donated by PrevEntry
   of the doubly linked list.
 
   @param[in, out]  PrevEntry             The previous entry to insert after.
   @param[in, out]  NewEntry              The new entry to insert.
@@ -1041,20 +1082,24 @@ EFIAPI
 NetListInsertAfter (
   IN OUT LIST_ENTRY         *PrevEntry,
   IN OUT LIST_ENTRY         *NewEntry
   )
 {
+  ASSERT (PrevEntry != NULL && NewEntry != NULL);
+
   NewEntry->BackLink                = PrevEntry;
   NewEntry->ForwardLink             = PrevEntry->ForwardLink;
   PrevEntry->ForwardLink->BackLink  = NewEntry;
   PrevEntry->ForwardLink            = NewEntry;
 }
 
 
 /**
   Insert a new node entry before a designated node entry of a doubly linked list.
 
+  ASSERT if PostEntry or NewEntry is NULL.
+
   Inserts a new node entry donated by NewEntry after the node entry donated by PostEntry
   of the doubly linked list.
 
   @param[in, out]  PostEntry             The entry to insert before.
   @param[in, out]  NewEntry              The new entry to insert.
@@ -1065,10 +1110,12 @@ EFIAPI
 NetListInsertBefore (
   IN OUT LIST_ENTRY     *PostEntry,
   IN OUT LIST_ENTRY     *NewEntry
   )
 {
+  ASSERT (PostEntry != NULL && NewEntry != NULL);
+
   NewEntry->ForwardLink             = PostEntry;
   NewEntry->BackLink                = PostEntry->BackLink;
   PostEntry->BackLink->ForwardLink  = NewEntry;
   PostEntry->BackLink               = NewEntry;
 }
@@ -1263,11 +1310,10 @@ NetMapClean (
 
   If the number of the <Key, Value> pairs in the netmap is zero, return TRUE.
 
   If Map is NULL, then ASSERT().
 
-
   @param[in]  Map                   The net map to test.
 
   @return TRUE if the netmap is empty, otherwise FALSE.
 
 **/
@@ -1283,10 +1329,12 @@ NetMapIsEmpty (
 
 
 /**
   Return the number of the <Key, Value> pairs in the netmap.
 
+  If Map is NULL, then ASSERT().
+
   @param[in]  Map                   The netmap to get the entry number.
 
   @return The entry number in the netmap.
 
 **/
@@ -1294,10 +1342,11 @@ UINTN
 EFIAPI
 NetMapGetCount (
   IN NET_MAP                *Map
   )
 {
+  ASSERT (Map != NULL);
   return Map->Count;
 }
 
 
 /**
@@ -1358,10 +1407,11 @@ NetMapAllocItem (
   Allocate an item to save the <Key, Value> pair and add corresponding node entry
   to the beginning of the Used doubly linked list. The number of the <Key, Value>
   pairs in the netmap increase by 1.
 
   If Map is NULL, then ASSERT().
+  If Key is NULL, then ASSERT().
 
   @param[in, out]  Map                   The netmap to insert into.
   @param[in]       Key                   The user's key.
   @param[in]       Value                 The user's value for the key.
 
@@ -1377,11 +1427,11 @@ NetMapInsertHead (
   IN VOID                   *Value    OPTIONAL
   )
 {
   NET_MAP_ITEM              *Item;
 
-  ASSERT (Map != NULL);
+  ASSERT (Map != NULL && Key != NULL);
 
   Item = NetMapAllocItem (Map);
 
   if (Item == NULL) {
     return EFI_OUT_OF_RESOURCES;
@@ -1402,10 +1452,11 @@ NetMapInsertHead (
   Allocate an item to save the <Key, Value> pair and add corresponding node entry
   to the tail of the Used doubly linked list. The number of the <Key, Value>
   pairs in the netmap increase by 1.
 
   If Map is NULL, then ASSERT().
+  If Key is NULL, then ASSERT().
 
   @param[in, out]  Map                   The netmap to insert into.
   @param[in]       Key                   The user's key.
   @param[in]       Value                 The user's value for the key.
 
@@ -1421,11 +1472,11 @@ NetMapInsertTail (
   IN VOID                   *Value    OPTIONAL
   )
 {
   NET_MAP_ITEM              *Item;
 
-  ASSERT (Map != NULL);
+  ASSERT (Map != NULL && Key != NULL);
 
   Item = NetMapAllocItem (Map);
 
   if (Item == NULL) {
     return EFI_OUT_OF_RESOURCES;
@@ -1442,10 +1493,13 @@ NetMapInsertTail (
 
 
 /**
   Check whether the item is in the Map and return TRUE if it is.
 
+  If Map is NULL, then ASSERT().
+  If Item is NULL, then ASSERT().
+
   @param[in]  Map                   The netmap to search within.
   @param[in]  Item                  The item to search.
 
   @return TRUE if the item is in the netmap, otherwise FALSE.
 
@@ -1456,10 +1510,12 @@ NetItemInMap (
   IN NET_MAP_ITEM           *Item
   )
 {
   LIST_ENTRY            *ListEntry;
 
+  ASSERT (Map != NULL && Item != NULL);
+
   NET_LIST_FOR_EACH (ListEntry, &Map->Used) {
     if (ListEntry == &Item->Link) {
       return TRUE;
     }
   }
@@ -1473,10 +1529,11 @@ NetItemInMap (
 
   Iterate the Used doubly linked list of the netmap to get every item. Compare the key of every
   item with the key to search. It returns the point to the item contains the Key if found.
 
   If Map is NULL, then ASSERT().
+  If Key is NULL, then ASSERT().
 
   @param[in]  Map                   The netmap to search within.
   @param[in]  Key                   The key to search.
 
   @return The point to the item contains the Key, or NULL if Key isn't in the map.
@@ -1490,11 +1547,11 @@ NetMapFindKey (
   )
 {
   LIST_ENTRY              *Entry;
   NET_MAP_ITEM            *Item;
 
-  ASSERT (Map != NULL);
+  ASSERT (Map != NULL && Key != NULL);
 
   NET_LIST_FOR_EACH (Entry, &Map->Used) {
     Item = NET_LIST_USER_STRUCT (Entry, NET_MAP_ITEM, Link);
 
     if (Item->Key == Key) {
@@ -2093,10 +2150,13 @@ NetLibGetVlanHandle (
 }
 
 /**
   Get MAC address associated with the network service handle.
 
+  If MacAddress is NULL, then ASSERT().
+  If AddressSize is NULL, then ASSERT().
+
   There should be MNP Service Binding Protocol installed on the input ServiceHandle.
   If SNP is installed on the ServiceHandle or its parent handle, MAC address will
   be retrieved from SNP. If no SNP found, try to get SNP mode data use MNP.
 
   @param[in]   ServiceHandle    The handle where network service binding protocols are
@@ -2197,10 +2257,12 @@ NetLibGetMacAddress (
 
 /**
   Convert MAC address of the NIC associated with specified Service Binding Handle
   to a unicode string. Callers are responsible for freeing the string storage.
 
+  If MacString is NULL, then ASSERT().
+
   Locate simple network protocol associated with the Service Binding Handle and
   get the mac address from SNP. Then convert the mac address into a unicode
   string. It takes 2 unicode characters to represent a 1 byte binary buffer.
   Plus one unicode character for the null-terminator.
 
@@ -2296,10 +2358,12 @@ NetLibGetMacString (
 }
 
 /**
   Detect media status for specified network device.
 
+  If MediaPresent is NULL, then ASSERT().
+
   The underlying UNDI driver may or may not support reporting media status from
   GET_STATUS command (PXE_STATFLAGS_GET_STATUS_NO_MEDIA_SUPPORTED). This routine
   will try to invoke Snp->GetStatus() to get the media status: if media already
   present, it return directly; if media not present, it will stop SNP and then
   restart SNP to get the latest media status, this give chance to get the correct
@@ -2404,10 +2468,14 @@ NetLibDetectMedia (
       MCastFilter = AllocateCopyPool (
                       MCastFilterCount * sizeof (EFI_MAC_ADDRESS),
                       Snp->Mode->MCastFilter
                       );
       ASSERT (MCastFilter != NULL);
+      if (MCastFilter == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto Exit;
+      }
 
       ResetMCastFilters = FALSE;
     }
 
     //
@@ -2735,10 +2803,12 @@ ON_EXIT:
 }
 
 /**
   Create an IPv4 device path node.
 
+  If Node is NULL, then ASSERT().
+
   The header type of IPv4 device path node is MESSAGING_DEVICE_PATH.
   The header subtype of IPv4 device path node is MSG_IPv4_DP.
   Get other info from parameters to make up the whole IPv4 device path node.
 
   @param[in, out]  Node                  Pointer to the IPv4 device path node.
@@ -2762,10 +2832,12 @@ NetLibCreateIPv4DPathNode (
   IN UINT16                RemotePort,
   IN UINT16                Protocol,
   IN BOOLEAN               UseDefaultAddress
   )
 {
+  ASSERT (Node != NULL);
+
   Node->Header.Type    = MESSAGING_DEVICE_PATH;
   Node->Header.SubType = MSG_IPv4_DP;
   SetDevicePathNodeLength (&Node->Header, sizeof (IPv4_DEVICE_PATH));
 
   CopyMem (&Node->LocalIpAddress, &LocalIp, sizeof (EFI_IPv4_ADDRESS));
@@ -2792,10 +2864,14 @@ NetLibCreateIPv4DPathNode (
 }
 
 /**
   Create an IPv6 device path node.
 
+  If Node is NULL, then ASSERT().
+  If LocalIp is NULL, then ASSERT().
+  If RemoteIp is NULL, then ASSERT().
+
   The header type of IPv6 device path node is MESSAGING_DEVICE_PATH.
   The header subtype of IPv6 device path node is MSG_IPv6_DP.
   Get other info from parameters to make up the whole IPv6 device path node.
 
   @param[in, out]  Node                  Pointer to the IPv6 device path node.
@@ -2817,10 +2893,12 @@ NetLibCreateIPv6DPathNode (
   IN EFI_IPv6_ADDRESS      *RemoteIp,
   IN UINT16                RemotePort,
   IN UINT16                Protocol
   )
 {
+  ASSERT (Node != NULL && LocalIp != NULL && RemoteIp != NULL);
+
   Node->Header.Type    = MESSAGING_DEVICE_PATH;
   Node->Header.SubType = MSG_IPv6_DP;
   SetDevicePathNodeLength (&Node->Header, sizeof (IPv6_DEVICE_PATH));
 
   CopyMem (&Node->LocalIpAddress, LocalIp, sizeof (EFI_IPv6_ADDRESS));
@@ -2841,10 +2919,12 @@ NetLibCreateIPv6DPathNode (
 }
 
 /**
   Find the UNDI/SNP handle from controller and protocol GUID.
 
+  If ProtocolGuid is NULL, then ASSERT().
+
   For example, IP will open a MNP child to transmit/receive
   packets, when MNP is stopped, IP should also be stopped. IP
   needs to find its own private data which is related the IP's
   service binding instance that is install on UNDI/SNP handle.
   Now, the controller is either a MNP or ARP child handle. But
@@ -2868,10 +2948,12 @@ NetLibGetNicHandle (
   EFI_HANDLE                          Handle;
   EFI_STATUS                          Status;
   UINTN                               OpenCount;
   UINTN                               Index;
 
+  ASSERT (ProtocolGuid != NULL);
+
   Status = gBS->OpenProtocolInformation (
                   Controller,
                   ProtocolGuid,
                   &OpenBuffer,
                   &OpenCount
@@ -3149,10 +3231,12 @@ NetLibIp6ToStr (
 }
 
 /**
   This function obtains the system guid from the smbios table.
 
+  If SystemGuid is NULL, then ASSERT().
+
   @param[out]  SystemGuid     The pointer of the returned system guid.
 
   @retval EFI_SUCCESS         Successfully obtained the system guid.
   @retval EFI_NOT_FOUND       Did not find the SMBIOS table.
 
@@ -3168,10 +3252,12 @@ NetLibGetSystemGuid (
   SMBIOS_TABLE_3_0_ENTRY_POINT  *Smbios30Table;
   SMBIOS_STRUCTURE_POINTER      Smbios;
   SMBIOS_STRUCTURE_POINTER      SmbiosEnd;
   CHAR8                         *String;
 
+  ASSERT (SystemGuid != NULL);
+
   SmbiosTable = NULL;
   Status = EfiGetSystemConfigurationTable (&gEfiSmbios3TableGuid, (VOID **) &Smbios30Table);
   if (!(EFI_ERROR (Status) || Smbios30Table == NULL)) {
     Smbios.Hdr = (SMBIOS_STRUCTURE *) (UINTN) Smbios30Table->TableAddress;
     SmbiosEnd.Raw = (UINT8 *) (UINTN) (Smbios30Table->TableAddress + Smbios30Table->TableMaximumSize);
@@ -3234,11 +3320,14 @@ NetLibGetSystemGuid (
   } while (Smbios.Raw < SmbiosEnd.Raw);
   return EFI_NOT_FOUND;
 }
 
 /**
-  Create Dns QName according the queried domain name. 
+  Create Dns QName according the queried domain name.
+
+  If DomainName is NULL, then ASSERT().
+  
   QName is a domain name represented as a sequence of labels, 
   where each label consists of a length octet followed by that 
   number of octets. The QName terminates with the zero 
   length octet for the null label of the root. Caller should 
   take responsibility to free the buffer in returned pointer.
@@ -3260,10 +3349,12 @@ NetLibCreateDnsQName (
   CHAR8                 *Header;
   CHAR8                 *Tail;
   UINTN                 Len;
   UINTN                 Index;
 
+  ASSERT (DomainName != NULL);
+
   QueryName     = NULL;
   QueryNameSize = 0;
   Header        = NULL;
   Tail          = NULL;
 
-- 
1.9.5.msysgit.1

Re: [Patch 2/3] MdeModulePkg/DxeNetLib: Add parameter check and ASSERT handling.

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>

> -----Original Message-----
> From: Wang, Fan
> Sent: Wednesday, January 3, 2018 10:32 AM
> To: edk2-devel@lists.01.org
> Cc: Wang, Fan <fan.wang@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ye,
> Ting <ting.ye@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 2/3] MdeModulePkg/DxeNetLib: Add parameter check and
> ASSERT handling.
> 
> From: Wang Fan <fan.wang@intel.com>
> 
> * Library API should check the input parameters before use, or
>   ASSERT to tell it has to meet some requirements. But in DxeNetLib,
>   not all functions follows this rule.
> * ASSERT shouldn't be used as error handling, add some handling code
>   for errors.
> * Add some ASSERT commence in function notes.
> 
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 119
> +++++++++++++++++++++++++----
>  1 file changed, 105 insertions(+), 14 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index 0f00f79..90f17b7 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Network library.
> 
> -Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
>  (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>  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
>  http://opensource.org/licenses/bsd-license.php
> @@ -196,10 +196,11 @@ SyslogLocateSnp (
>    Transmit a syslog packet synchronously through SNP. The Packet
>    already has the ethernet header prepended. This function should
>    fill in the source MAC because it will try to locate a SNP each
>    time it is called to avoid the problem if SNP is unloaded.
>    This code snip is copied from MNP.
> +  If Packet is NULL, then ASSERT().
> 
>    @param[in] Packet          The Syslog packet
>    @param[in] Length          The length of the packet
> 
>    @retval EFI_DEVICE_ERROR   Failed to locate a usable SNP protocol
> @@ -217,10 +218,12 @@ SyslogSendPacket (
>    ETHER_HEAD                  *Ether;
>    EFI_STATUS                  Status;
>    EFI_EVENT                   TimeoutEvent;
>    UINT8                       *TxBuf;
> 
> +  ASSERT (Packet != NULL);
> +
>    Snp = SyslogLocateSnp ();
> 
>    if (Snp == NULL) {
>      return EFI_DEVICE_ERROR;
>    }
> @@ -308,11 +311,11 @@ ON_EXIT:
>    @param[in]  Line      The line of code in the File that contains the current log
>    @param[in]  Message   The log message
>    @param[in]  BufLen    The lenght of the Buf
>    @param[out] Buf       The buffer to put the packet data
> 
> -  @return The length of the syslog packet built.
> +  @return The length of the syslog packet built, 0 represents no packet is
> built.
> 
>  **/
>  UINT32
>  SyslogBuildPacket (
>    IN  UINT32                Level,
> @@ -322,10 +325,11 @@ SyslogBuildPacket (
>    IN  UINT8                 *Message,
>    IN  UINT32                BufLen,
>    OUT CHAR8                 *Buf
>    )
>  {
> +  EFI_STATUS                Status;
>    ETHER_HEAD                *Ether;
>    IP4_HEAD                  *Ip4;
>    EFI_UDP_HEADER            *Udp4;
>    EFI_TIME                  Time;
>    UINT32                    Pri;
> @@ -377,12 +381,14 @@ SyslogBuildPacket (
> 
>    //
>    // Build the syslog message body with <PRI> Timestamp  machine module
> Message
>    //
>    Pri = ((NET_SYSLOG_FACILITY & 31) << 3) | (Level & 7);
> -  gRT->GetTime (&Time, NULL);
> -  ASSERT ((Time.Month <= 12) && (Time.Month >= 1));
> +  Status = gRT->GetTime (&Time, NULL);
> +  if (EFI_ERROR (Status)) {
> +    return 0;
> +  }
> 
>    //
>    // Use %a to format the ASCII strings, %s to format UNICODE strings
>    //
>    Len  = 0;
> @@ -437,10 +443,12 @@ SyslogBuildPacket (
>             __FILE__,
>             __LINE__,
>             NetDebugASPrint ("State transit to %a\n", Name)
>           )
> 
> +  If Format is NULL, then ASSERT().
> +
>    @param Format  The ASCII format string.
>    @param ...     The variable length parameter whose format is determined
>                   by the Format string.
> 
>    @return        The buffer containing the formatted message,
> @@ -455,10 +463,12 @@ NetDebugASPrint (
>    )
>  {
>    VA_LIST                   Marker;
>    CHAR8                     *Buf;
> 
> +  ASSERT (Format != NULL);
> +
>    Buf = (CHAR8 *) AllocatePool (NET_DEBUG_MSG_LEN);
> 
>    if (Buf == NULL) {
>      return NULL;
>    }
> @@ -481,11 +491,12 @@ NetDebugASPrint (
>    @param File     The file that contains the log.
>    @param Line     The exact line that contains the log.
>    @param Message  The user message to log.
> 
>    @retval EFI_INVALID_PARAMETER Any input parameter is invalid.
> -  @retval EFI_OUT_OF_RESOURCES  Failed to allocate memory for the
> packet
> +  @retval EFI_OUT_OF_RESOURCES  Failed to allocate memory for the
> packet.
> +  @retval EFI_DEVICE_ERROR      Device error occurs.
>    @retval EFI_SUCCESS           The log is discard because that it is more verbose
>                                  than the mNetDebugLevelMax. Or, it has been sent out.
>  **/
>  EFI_STATUS
>  EFIAPI
> @@ -502,11 +513,11 @@ NetDebugOutput (
>    EFI_STATUS                   Status;
> 
>    //
>    // Check whether the message should be sent out
>    //
> -  if (Message == NULL) {
> +  if (Message == NULL || File == NULL || Module == NULL) {
>      return EFI_INVALID_PARAMETER;
>    }
> 
>    if (Level > mNetDebugLevelMax) {
>      Status = EFI_SUCCESS;
> @@ -535,13 +546,17 @@ NetDebugOutput (
>            Line,
>            Message,
>            NET_SYSLOG_PACKET_LEN,
>            Packet
>            );
> +  if (Len == 0) {
> +    Status = EFI_DEVICE_ERROR;
> +  } else {
> +    mSyslogPacketSeq++;
> +    Status = SyslogSendPacket (Packet, Len);
> +  }
> 
> -  mSyslogPacketSeq++;
> -  Status = SyslogSendPacket (Packet, Len);
>    FreePool (Packet);
> 
>  ON_EXIT:
>    FreePool (Message);
>    return Status;
> @@ -673,10 +688,12 @@ NetIp4IsUnicast (
>  }
> 
>  /**
>    Check whether the incoming IPv6 address is a valid unicast address.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    If the address is a multicast address has binary 0xFF at the start, it is not
>    a valid unicast address. If the address is unspecified ::, it is not a valid
>    unicast address to be assigned to any node. If the address is loopback
> address
>    ::1, it is also not a valid unicast address to be assigned to any physical
>    interface.
> @@ -693,10 +710,12 @@ NetIp6IsValidUnicast (
>    )
>  {
>    UINT8 Byte;
>    UINT8 Index;
> 
> +  ASSERT (Ip6 != NULL);
> +
>    if (Ip6->Addr[0] == 0xFF) {
>      return FALSE;
>    }
> 
>    for (Index = 0; Index < 15; Index++) {
> @@ -715,10 +734,12 @@ NetIp6IsValidUnicast (
>  }
> 
>  /**
>    Check whether the incoming Ipv6 address is the unspecified address or not.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    @param[in] Ip6   - Ip6 address, in network order.
> 
>    @retval TRUE     - Yes, unspecified
>    @retval FALSE    - No
> 
> @@ -729,10 +750,12 @@ NetIp6IsUnspecifiedAddr (
>    IN EFI_IPv6_ADDRESS       *Ip6
>    )
>  {
>    UINT8 Index;
> 
> +  ASSERT (Ip6 != NULL);
> +
>    for (Index = 0; Index < 16; Index++) {
>      if (Ip6->Addr[Index] != 0) {
>        return FALSE;
>      }
>    }
> @@ -741,10 +764,12 @@ NetIp6IsUnspecifiedAddr (
>  }
> 
>  /**
>    Check whether the incoming Ipv6 address is a link-local address.
> 
> +  ASSERT if Ip6 is NULL.
> +
>    @param[in] Ip6   - Ip6 address, in network order.
> 
>    @retval TRUE  - Yes, link-local address
>    @retval FALSE - No
> 
> @@ -777,10 +802,13 @@ NetIp6IsLinkLocalAddr (
>  }
> 
>  /**
>    Check whether the Ipv6 address1 and address2 are on the connected
> network.
> 
> +  ASSERT if Ip1 or Ip2 is NULL.
> +  ASSERT if PrefixLength exceeds IP6_PREFIX_MAX.
> +
>    @param[in] Ip1          - Ip6 address1, in network order.
>    @param[in] Ip2          - Ip6 address2, in network order.
>    @param[in] PrefixLength - The prefix length of the checking net.
> 
>    @retval TRUE            - Yes, connected.
> @@ -813,11 +841,10 @@ NetIp6IsNetEqual (
>    }
> 
>    if (Bit > 0) {
>      Mask = (UINT8) (0xFF << (8 - Bit));
> 
> -    ASSERT (Byte < 16);
>      if ((Ip1->Addr[Byte] & Mask) != (Ip2->Addr[Byte] & Mask)) {
>        return FALSE;
>      }
>    }
> 
> @@ -826,10 +853,12 @@ NetIp6IsNetEqual (
> 
> 
>  /**
>    Switches the endianess of an IPv6 address
> 
> +  ASSERT if Ip6 is NULL.
> +
>    This function swaps the bytes in a 128-bit IPv6 address to switch the value
>    from little endian to big endian or vice versa. The byte swapped value is
>    returned.
> 
>    @param  Ip6 Points to an IPv6 address
> @@ -844,10 +873,12 @@ Ip6Swap128 (
>    )
>  {
>    UINT64 High;
>    UINT64 Low;
> 
> +  ASSERT (Ip6 != NULL);
> +
>    CopyMem (&High, Ip6, sizeof (UINT64));
>    CopyMem (&Low, &Ip6->Addr[8], sizeof (UINT64));
> 
>    High = SwapBytes64 (High);
>    Low  = SwapBytes64 (Low);
> @@ -891,10 +922,12 @@ NetRandomInitSeed (
> 
> 
>  /**
>    Extract a UINT32 from a byte stream.
> 
> +  ASSERT if Buf is NULL.
> +
>    Copy a UINT32 from a byte stream, then converts it from Network
>    byte order to host byte order. Use this function to avoid alignment error.
> 
>    @param[in]  Buf                 The buffer to extract the UINT32.
> 
> @@ -907,18 +940,22 @@ NetGetUint32 (
>    IN UINT8                  *Buf
>    )
>  {
>    UINT32                    Value;
> 
> +  ASSERT (Buf != NULL);
> +
>    CopyMem (&Value, Buf, sizeof (UINT32));
>    return NTOHL (Value);
>  }
> 
> 
>  /**
>    Put a UINT32 to the byte stream in network byte order.
> 
> +  ASSERT if Buf is NULL.
> +
>    Converts a UINT32 from host byte order to network byte order. Then copy
> it to the
>    byte stream.
> 
>    @param[in, out]  Buf          The buffer to put the UINT32.
>    @param[in]       Data         The data to be converted and put into the byte
> stream.
> @@ -929,10 +966,12 @@ EFIAPI
>  NetPutUint32 (
>    IN OUT UINT8                 *Buf,
>    IN     UINT32                Data
>    )
>  {
> +  ASSERT (Buf != NULL);
> +
>    Data = HTONL (Data);
>    CopyMem (Buf, &Data, sizeof (UINT32));
>  }
> 
> 
> @@ -1027,10 +1066,12 @@ NetListRemoveTail (
> 
> 
>  /**
>    Insert a new node entry after a designated node entry of a doubly linked
> list.
> 
> +  ASSERT if PrevEntry or NewEntry is NULL.
> +
>    Inserts a new node entry donated by NewEntry after the node entry
> donated by PrevEntry
>    of the doubly linked list.
> 
>    @param[in, out]  PrevEntry             The previous entry to insert after.
>    @param[in, out]  NewEntry              The new entry to insert.
> @@ -1041,20 +1082,24 @@ EFIAPI
>  NetListInsertAfter (
>    IN OUT LIST_ENTRY         *PrevEntry,
>    IN OUT LIST_ENTRY         *NewEntry
>    )
>  {
> +  ASSERT (PrevEntry != NULL && NewEntry != NULL);
> +
>    NewEntry->BackLink                = PrevEntry;
>    NewEntry->ForwardLink             = PrevEntry->ForwardLink;
>    PrevEntry->ForwardLink->BackLink  = NewEntry;
>    PrevEntry->ForwardLink            = NewEntry;
>  }
> 
> 
>  /**
>    Insert a new node entry before a designated node entry of a doubly linked
> list.
> 
> +  ASSERT if PostEntry or NewEntry is NULL.
> +
>    Inserts a new node entry donated by NewEntry after the node entry
> donated by PostEntry
>    of the doubly linked list.
> 
>    @param[in, out]  PostEntry             The entry to insert before.
>    @param[in, out]  NewEntry              The new entry to insert.
> @@ -1065,10 +1110,12 @@ EFIAPI
>  NetListInsertBefore (
>    IN OUT LIST_ENTRY     *PostEntry,
>    IN OUT LIST_ENTRY     *NewEntry
>    )
>  {
> +  ASSERT (PostEntry != NULL && NewEntry != NULL);
> +
>    NewEntry->ForwardLink             = PostEntry;
>    NewEntry->BackLink                = PostEntry->BackLink;
>    PostEntry->BackLink->ForwardLink  = NewEntry;
>    PostEntry->BackLink               = NewEntry;
>  }
> @@ -1263,11 +1310,10 @@ NetMapClean (
> 
>    If the number of the <Key, Value> pairs in the netmap is zero, return TRUE.
> 
>    If Map is NULL, then ASSERT().
> 
> -
>    @param[in]  Map                   The net map to test.
> 
>    @return TRUE if the netmap is empty, otherwise FALSE.
> 
>  **/
> @@ -1283,10 +1329,12 @@ NetMapIsEmpty (
> 
> 
>  /**
>    Return the number of the <Key, Value> pairs in the netmap.
> 
> +  If Map is NULL, then ASSERT().
> +
>    @param[in]  Map                   The netmap to get the entry number.
> 
>    @return The entry number in the netmap.
> 
>  **/
> @@ -1294,10 +1342,11 @@ UINTN
>  EFIAPI
>  NetMapGetCount (
>    IN NET_MAP                *Map
>    )
>  {
> +  ASSERT (Map != NULL);
>    return Map->Count;
>  }
> 
> 
>  /**
> @@ -1358,10 +1407,11 @@ NetMapAllocItem (
>    Allocate an item to save the <Key, Value> pair and add corresponding node
> entry
>    to the beginning of the Used doubly linked list. The number of the <Key,
> Value>
>    pairs in the netmap increase by 1.
> 
>    If Map is NULL, then ASSERT().
> +  If Key is NULL, then ASSERT().
> 
>    @param[in, out]  Map                   The netmap to insert into.
>    @param[in]       Key                   The user's key.
>    @param[in]       Value                 The user's value for the key.
> 
> @@ -1377,11 +1427,11 @@ NetMapInsertHead (
>    IN VOID                   *Value    OPTIONAL
>    )
>  {
>    NET_MAP_ITEM              *Item;
> 
> -  ASSERT (Map != NULL);
> +  ASSERT (Map != NULL && Key != NULL);
> 
>    Item = NetMapAllocItem (Map);
> 
>    if (Item == NULL) {
>      return EFI_OUT_OF_RESOURCES;
> @@ -1402,10 +1452,11 @@ NetMapInsertHead (
>    Allocate an item to save the <Key, Value> pair and add corresponding node
> entry
>    to the tail of the Used doubly linked list. The number of the <Key, Value>
>    pairs in the netmap increase by 1.
> 
>    If Map is NULL, then ASSERT().
> +  If Key is NULL, then ASSERT().
> 
>    @param[in, out]  Map                   The netmap to insert into.
>    @param[in]       Key                   The user's key.
>    @param[in]       Value                 The user's value for the key.
> 
> @@ -1421,11 +1472,11 @@ NetMapInsertTail (
>    IN VOID                   *Value    OPTIONAL
>    )
>  {
>    NET_MAP_ITEM              *Item;
> 
> -  ASSERT (Map != NULL);
> +  ASSERT (Map != NULL && Key != NULL);
> 
>    Item = NetMapAllocItem (Map);
> 
>    if (Item == NULL) {
>      return EFI_OUT_OF_RESOURCES;
> @@ -1442,10 +1493,13 @@ NetMapInsertTail (
> 
> 
>  /**
>    Check whether the item is in the Map and return TRUE if it is.
> 
> +  If Map is NULL, then ASSERT().
> +  If Item is NULL, then ASSERT().
> +
>    @param[in]  Map                   The netmap to search within.
>    @param[in]  Item                  The item to search.
> 
>    @return TRUE if the item is in the netmap, otherwise FALSE.
> 
> @@ -1456,10 +1510,12 @@ NetItemInMap (
>    IN NET_MAP_ITEM           *Item
>    )
>  {
>    LIST_ENTRY            *ListEntry;
> 
> +  ASSERT (Map != NULL && Item != NULL);
> +
>    NET_LIST_FOR_EACH (ListEntry, &Map->Used) {
>      if (ListEntry == &Item->Link) {
>        return TRUE;
>      }
>    }
> @@ -1473,10 +1529,11 @@ NetItemInMap (
> 
>    Iterate the Used doubly linked list of the netmap to get every item.
> Compare the key of every
>    item with the key to search. It returns the point to the item contains the
> Key if found.
> 
>    If Map is NULL, then ASSERT().
> +  If Key is NULL, then ASSERT().
> 
>    @param[in]  Map                   The netmap to search within.
>    @param[in]  Key                   The key to search.
> 
>    @return The point to the item contains the Key, or NULL if Key isn't in the
> map.
> @@ -1490,11 +1547,11 @@ NetMapFindKey (
>    )
>  {
>    LIST_ENTRY              *Entry;
>    NET_MAP_ITEM            *Item;
> 
> -  ASSERT (Map != NULL);
> +  ASSERT (Map != NULL && Key != NULL);
> 
>    NET_LIST_FOR_EACH (Entry, &Map->Used) {
>      Item = NET_LIST_USER_STRUCT (Entry, NET_MAP_ITEM, Link);
> 
>      if (Item->Key == Key) {
> @@ -2093,10 +2150,13 @@ NetLibGetVlanHandle (
>  }
> 
>  /**
>    Get MAC address associated with the network service handle.
> 
> +  If MacAddress is NULL, then ASSERT().
> +  If AddressSize is NULL, then ASSERT().
> +
>    There should be MNP Service Binding Protocol installed on the input
> ServiceHandle.
>    If SNP is installed on the ServiceHandle or its parent handle, MAC address
> will
>    be retrieved from SNP. If no SNP found, try to get SNP mode data use MNP.
> 
>    @param[in]   ServiceHandle    The handle where network service binding
> protocols are
> @@ -2197,10 +2257,12 @@ NetLibGetMacAddress (
> 
>  /**
>    Convert MAC address of the NIC associated with specified Service Binding
> Handle
>    to a unicode string. Callers are responsible for freeing the string storage.
> 
> +  If MacString is NULL, then ASSERT().
> +
>    Locate simple network protocol associated with the Service Binding Handle
> and
>    get the mac address from SNP. Then convert the mac address into a
> unicode
>    string. It takes 2 unicode characters to represent a 1 byte binary buffer.
>    Plus one unicode character for the null-terminator.
> 
> @@ -2296,10 +2358,12 @@ NetLibGetMacString (
>  }
> 
>  /**
>    Detect media status for specified network device.
> 
> +  If MediaPresent is NULL, then ASSERT().
> +
>    The underlying UNDI driver may or may not support reporting media status
> from
>    GET_STATUS command
> (PXE_STATFLAGS_GET_STATUS_NO_MEDIA_SUPPORTED). This routine
>    will try to invoke Snp->GetStatus() to get the media status: if media already
>    present, it return directly; if media not present, it will stop SNP and then
>    restart SNP to get the latest media status, this give chance to get the
> correct
> @@ -2404,10 +2468,14 @@ NetLibDetectMedia (
>        MCastFilter = AllocateCopyPool (
>                        MCastFilterCount * sizeof (EFI_MAC_ADDRESS),
>                        Snp->Mode->MCastFilter
>                        );
>        ASSERT (MCastFilter != NULL);
> +      if (MCastFilter == NULL) {
> +        Status = EFI_OUT_OF_RESOURCES;
> +        goto Exit;
> +      }
> 
>        ResetMCastFilters = FALSE;
>      }
> 
>      //
> @@ -2735,10 +2803,12 @@ ON_EXIT:
>  }
> 
>  /**
>    Create an IPv4 device path node.
> 
> +  If Node is NULL, then ASSERT().
> +
>    The header type of IPv4 device path node is MESSAGING_DEVICE_PATH.
>    The header subtype of IPv4 device path node is MSG_IPv4_DP.
>    Get other info from parameters to make up the whole IPv4 device path
> node.
> 
>    @param[in, out]  Node                  Pointer to the IPv4 device path node.
> @@ -2762,10 +2832,12 @@ NetLibCreateIPv4DPathNode (
>    IN UINT16                RemotePort,
>    IN UINT16                Protocol,
>    IN BOOLEAN               UseDefaultAddress
>    )
>  {
> +  ASSERT (Node != NULL);
> +
>    Node->Header.Type    = MESSAGING_DEVICE_PATH;
>    Node->Header.SubType = MSG_IPv4_DP;
>    SetDevicePathNodeLength (&Node->Header, sizeof (IPv4_DEVICE_PATH));
> 
>    CopyMem (&Node->LocalIpAddress, &LocalIp, sizeof (EFI_IPv4_ADDRESS));
> @@ -2792,10 +2864,14 @@ NetLibCreateIPv4DPathNode (
>  }
> 
>  /**
>    Create an IPv6 device path node.
> 
> +  If Node is NULL, then ASSERT().
> +  If LocalIp is NULL, then ASSERT().
> +  If RemoteIp is NULL, then ASSERT().
> +
>    The header type of IPv6 device path node is MESSAGING_DEVICE_PATH.
>    The header subtype of IPv6 device path node is MSG_IPv6_DP.
>    Get other info from parameters to make up the whole IPv6 device path
> node.
> 
>    @param[in, out]  Node                  Pointer to the IPv6 device path node.
> @@ -2817,10 +2893,12 @@ NetLibCreateIPv6DPathNode (
>    IN EFI_IPv6_ADDRESS      *RemoteIp,
>    IN UINT16                RemotePort,
>    IN UINT16                Protocol
>    )
>  {
> +  ASSERT (Node != NULL && LocalIp != NULL && RemoteIp != NULL);
> +
>    Node->Header.Type    = MESSAGING_DEVICE_PATH;
>    Node->Header.SubType = MSG_IPv6_DP;
>    SetDevicePathNodeLength (&Node->Header, sizeof (IPv6_DEVICE_PATH));
> 
>    CopyMem (&Node->LocalIpAddress, LocalIp, sizeof (EFI_IPv6_ADDRESS));
> @@ -2841,10 +2919,12 @@ NetLibCreateIPv6DPathNode (
>  }
> 
>  /**
>    Find the UNDI/SNP handle from controller and protocol GUID.
> 
> +  If ProtocolGuid is NULL, then ASSERT().
> +
>    For example, IP will open a MNP child to transmit/receive
>    packets, when MNP is stopped, IP should also be stopped. IP
>    needs to find its own private data which is related the IP's
>    service binding instance that is install on UNDI/SNP handle.
>    Now, the controller is either a MNP or ARP child handle. But
> @@ -2868,10 +2948,12 @@ NetLibGetNicHandle (
>    EFI_HANDLE                          Handle;
>    EFI_STATUS                          Status;
>    UINTN                               OpenCount;
>    UINTN                               Index;
> 
> +  ASSERT (ProtocolGuid != NULL);
> +
>    Status = gBS->OpenProtocolInformation (
>                    Controller,
>                    ProtocolGuid,
>                    &OpenBuffer,
>                    &OpenCount
> @@ -3149,10 +3231,12 @@ NetLibIp6ToStr (
>  }
> 
>  /**
>    This function obtains the system guid from the smbios table.
> 
> +  If SystemGuid is NULL, then ASSERT().
> +
>    @param[out]  SystemGuid     The pointer of the returned system guid.
> 
>    @retval EFI_SUCCESS         Successfully obtained the system guid.
>    @retval EFI_NOT_FOUND       Did not find the SMBIOS table.
> 
> @@ -3168,10 +3252,12 @@ NetLibGetSystemGuid (
>    SMBIOS_TABLE_3_0_ENTRY_POINT  *Smbios30Table;
>    SMBIOS_STRUCTURE_POINTER      Smbios;
>    SMBIOS_STRUCTURE_POINTER      SmbiosEnd;
>    CHAR8                         *String;
> 
> +  ASSERT (SystemGuid != NULL);
> +
>    SmbiosTable = NULL;
>    Status = EfiGetSystemConfigurationTable (&gEfiSmbios3TableGuid, (VOID
> **) &Smbios30Table);
>    if (!(EFI_ERROR (Status) || Smbios30Table == NULL)) {
>      Smbios.Hdr = (SMBIOS_STRUCTURE *) (UINTN) Smbios30Table-
> >TableAddress;
>      SmbiosEnd.Raw = (UINT8 *) (UINTN) (Smbios30Table->TableAddress +
> Smbios30Table->TableMaximumSize);
> @@ -3234,11 +3320,14 @@ NetLibGetSystemGuid (
>    } while (Smbios.Raw < SmbiosEnd.Raw);
>    return EFI_NOT_FOUND;
>  }
> 
>  /**
> -  Create Dns QName according the queried domain name.
> +  Create Dns QName according the queried domain name.
> +
> +  If DomainName is NULL, then ASSERT().
> +
>    QName is a domain name represented as a sequence of labels,
>    where each label consists of a length octet followed by that
>    number of octets. The QName terminates with the zero
>    length octet for the null label of the root. Caller should
>    take responsibility to free the buffer in returned pointer.
> @@ -3260,10 +3349,12 @@ NetLibCreateDnsQName (
>    CHAR8                 *Header;
>    CHAR8                 *Tail;
>    UINTN                 Len;
>    UINTN                 Index;
> 
> +  ASSERT (DomainName != NULL);
> +
>    QueryName     = NULL;
>    QueryNameSize = 0;
>    Header        = NULL;
>    Tail          = NULL;
> 
> --
> 1.9.5.msysgit.1

[Patch 3/3] MdeModulePkg/DxeNetLib: Fix an error in packet length counting.

[fanwang2]

From: Wang Fan <fan.wang@intel.com>

* In old implementation, the operation len-- assumes AsciiSPrint()
  has counted NULL terminator, and it's not correct. This patch is
  to fix this issue.

Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Ye Ting <ting.ye@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan <fan.wang@intel.com>
---
 MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
index 90f17b7..cbce28f 100644
--- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
+++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
@@ -401,22 +401,21 @@ SyslogBuildPacket (
                     Time.Day,
                     Time.Hour,
                     Time.Minute,
                     Time.Second
                     );
-  Len--;
 
   Len += (UINT32) AsciiSPrint (
                     Buf + Len,
                     BufLen - Len,
                     "Tiano %a: %a (Line: %d File: %a)",
                     Module,
                     Message,
                     Line,
                     File
                     );
-  Len--;
+  Len ++;
 
   //
   // OK, patch the IP length/checksum and UDP length fields.
   //
   Len           += sizeof (EFI_UDP_HEADER);
-- 
1.9.5.msysgit.1

Re: [Patch 3/3] MdeModulePkg/DxeNetLib: Fix an error in packet length counting.

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>


> -----Original Message-----
> From: Wang, Fan
> Sent: Wednesday, January 3, 2018 10:32 AM
> To: edk2-devel@lists.01.org
> Cc: Wang, Fan <fan.wang@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ye,
> Ting <ting.ye@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [Patch 3/3] MdeModulePkg/DxeNetLib: Fix an error in packet length
> counting.
> 
> From: Wang Fan <fan.wang@intel.com>
> 
> * In old implementation, the operation len-- assumes AsciiSPrint()
>   has counted NULL terminator, and it's not correct. This patch is
>   to fix this issue.
> 
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan <fan.wang@intel.com>
> ---
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index 90f17b7..cbce28f 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -401,22 +401,21 @@ SyslogBuildPacket (
>                      Time.Day,
>                      Time.Hour,
>                      Time.Minute,
>                      Time.Second
>                      );
> -  Len--;
> 
>    Len += (UINT32) AsciiSPrint (
>                      Buf + Len,
>                      BufLen - Len,
>                      "Tiano %a: %a (Line: %d File: %a)",
>                      Module,
>                      Message,
>                      Line,
>                      File
>                      );
> -  Len--;
> +  Len ++;
> 
>    //
>    // OK, patch the IP length/checksum and UDP length fields.
>    //
>    Len           += sizeof (EFI_UDP_HEADER);
> --
> 1.9.5.msysgit.1