Reviewed-by: Jiaxin Wu <jiaxin...@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.w...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Ye, > Ting <ting...@intel.com>; Wu, Jiaxin <jiaxin...@intel.com> > Subject: [Patch 2/3] MdeModulePkg/DxeNetLib: Add parameter check and > ASSERT handling. > > From: Wang Fan <fan.w...@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...@intel.com> > Cc: Ye Ting <ting...@intel.com> > Cc: Jiaxin Wu <jiaxin...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Wang Fan <fan.w...@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
_______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
