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

[Wu, Jiaxin]

Reviewed-by: Jiaxin Wu 

> -Original Message-
> From: Wang, Fan
> Sent: Wednesday, January 3, 2018 10:32 AM
> To: edk2-devel@lists.01.org
> Cc: Wang, Fan ; Fu, Siyuan ; Ye,
> Ting ; Wu, Jiaxin 
> Subject: [Patch 2/3] MdeModulePkg/DxeNetLib: Add parameter check and
> ASSERT handling.
> 
> From: Wang Fan 
> 
> * 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 
> Cc: Ye Ting 
> Cc: Jiaxin Wu 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan 
> ---
>  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.
> +Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.
>  (C) Copyright 2015 Hewlett Packard Enterprise Development LP
>  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]  BufLenThe 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  UINT32Level,
> @@ -322,10 +325,11 @@ SyslogBuildPacket (
>IN  UINT8 *Message,
>IN  UINT32BufLen,
>OUT CHAR8 *Buf
>)
>  {
> +  EFI_STATUSStatus;
>ETHER_HEAD*Ether;
>IP4_HEAD  *Ip4;
>EFI_UDP_HEADER*Udp4;
>EFI_TIME  Time;
>UINT32Pri;
> @@ -377,12 +381,14 @@ SyslogBuildPacket (
> 
>//
>// Build the syslog message body with  Timestamp  machine module
> Message
>//
>Pri = ((NET_SYSLOG_FACILITY & 31) << 3) | (Level & 7);
> -  gRT->GetTime (, NULL);
> -  ASSERT ((Time.Month <= 12) && (Time.Month >= 1));
> +  Status = gRT->GetTime (, 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.
> 
>@returnThe 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 

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

[fanwang2]

From: Wang Fan 

* 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 
Cc: Ye Ting 
Cc: Jiaxin Wu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 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.
+Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.
 (C) Copyright 2015 Hewlett Packard Enterprise Development LP
 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]  BufLenThe 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  UINT32Level,
@@ -322,10 +325,11 @@ SyslogBuildPacket (
   IN  UINT8 *Message,
   IN  UINT32BufLen,
   OUT CHAR8 *Buf
   )
 {
+  EFI_STATUSStatus;
   ETHER_HEAD*Ether;
   IP4_HEAD  *Ip4;
   EFI_UDP_HEADER*Udp4;
   EFI_TIME  Time;
   UINT32Pri;
@@ -377,12 +381,14 @@ SyslogBuildPacket (
 
   //
   // Build the syslog message body with  Timestamp  machine module Message
   //
   Pri = ((NET_SYSLOG_FACILITY & 31) << 3) | (Level & 7);
-  gRT->GetTime (, NULL);
-  ASSERT ((Time.Month <= 12) && (Time.Month >= 1));
+  Status = gRT->GetTime (, 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.
 
   @returnThe 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