[PATCH v2 0/6] Refine code logics to prevent possible mis-reports by static code checkers

[PATCH v2 0/6] Refine code logics to prevent possible mis-reports by static code checkers

[Hao Wu]

V2:
Patch 1 of the V1 series has an issue that APIs [Ascii]StrnLens will
access (MaxSize + 1) characters of String if there is no null character in
the first MaxSize characters in String. However, according to the comments
of these APIs, only MaxSize characters of String shall be accessed.

V2 addresses this issue. Also, 'r-b' tags have been added to the reviewed
patches of the series.


V1:
The series refines the loop logic (e.g. for, while) of some functions to
be more straightforward. This will help to prevent some possible
mis-reports by static code checkers

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Hao Wu (6):
  MdePkg/BaseLib: Refine (Ascii)StrnLenS functions logic
  MdePkg/BaseLib: Add an additional check within (Ascii)StrnCmp
  MdePkg/MemoryLib: Refine InternalMemSetMem16|32|64 functions logic
  MdeModulePkg/DxeNetLib: Rewrite NetblockChecksum function logic
  MdeModulePkg/UefiPxeBcDxe: Refine the CvtNum function logic
  NetworkPkg: Refine UintnToAscDecWithFormat functions logic

 MdeModulePkg/Library/DxeNetLib/NetBuffer.c             | 16 ++++++++--------
 .../Universal/Network/UefiPxeBcDxe/PxeBcSupport.c      |  5 ++---
 MdePkg/Library/BaseLib/SafeString.c                    | 16 ++++++++++++----
 MdePkg/Library/BaseLib/String.c                        |  4 +++-
 MdePkg/Library/BaseMemoryLib/MemLibGeneric.c           | 18 +++++++++---------
 MdePkg/Library/PeiMemoryLib/MemLibGeneric.c            | 18 +++++++++---------
 MdePkg/Library/UefiMemoryLib/MemLibGeneric.c           | 18 +++++++++---------
 NetworkPkg/HttpBootDxe/HttpBootSupport.c               |  5 ++---
 NetworkPkg/UefiPxeBcDxe/PxeBcSupport.c                 |  5 ++---
 9 files changed, 56 insertions(+), 49 deletions(-)

-- 
1.9.5.msysgit.0

[PATCH v2 1/6] MdePkg/BaseLib: Refine (Ascii)StrnLenS functions logic

[Hao Wu]

This commit refines the logic for AsciiStrnLenS and StrnLenS. It makes the
logic more straightforward to prevent possible mis-reports by static code
checkers.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdePkg/Library/BaseLib/SafeString.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Library/BaseLib/SafeString.c b/MdePkg/Library/BaseLib/SafeString.c
index ede2f4c..e4c0759 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -143,8 +143,12 @@ StrnLenS (
   // String then StrnLenS returns MaxSize. At most the first MaxSize characters of String shall
   // be accessed by StrnLenS.
   //
-  for (Length = 0; (Length < MaxSize) && (*String != 0); String++, Length++) {
-    ;
+  Length = 0;
+  while (String[Length] != 0) {
+    if (Length >= MaxSize - 1) {
+      return MaxSize;
+    }
+    Length++;
   }
   return Length;
 }
@@ -571,8 +575,12 @@ AsciiStrnLenS (
   // String then AsciiStrnLenS returns MaxSize. At most the first MaxSize characters of String shall
   // be accessed by AsciiStrnLenS.
   //
-  for (Length = 0; (Length < MaxSize) && (*String != 0); String++, Length++) {
-    ;
+  Length = 0;
+  while (String[Length] != 0) {
+    if (Length >= MaxSize - 1) {
+      return MaxSize;
+    }
+    Length++;
   }
   return Length;
 }
-- 
1.9.5.msysgit.0

Re: [PATCH v2 1/6] MdePkg/BaseLib: Refine (Ascii)StrnLenS functions logic

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Wu, Hao A
> Sent: Friday, December 16, 2016 11:38 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH v2 1/6] MdePkg/BaseLib: Refine (Ascii)StrnLenS functions
> logic
> 
> This commit refines the logic for AsciiStrnLenS and StrnLenS. It makes the
> logic more straightforward to prevent possible mis-reports by static code
> checkers.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  MdePkg/Library/BaseLib/SafeString.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/SafeString.c
> b/MdePkg/Library/BaseLib/SafeString.c
> index ede2f4c..e4c0759 100644
> --- a/MdePkg/Library/BaseLib/SafeString.c
> +++ b/MdePkg/Library/BaseLib/SafeString.c
> @@ -143,8 +143,12 @@ StrnLenS (
>    // String then StrnLenS returns MaxSize. At most the first MaxSize
> characters of String shall
>    // be accessed by StrnLenS.
>    //
> -  for (Length = 0; (Length < MaxSize) && (*String != 0); String++, Length++) {
> -    ;
> +  Length = 0;
> +  while (String[Length] != 0) {
> +    if (Length >= MaxSize - 1) {
> +      return MaxSize;
> +    }
> +    Length++;
>    }
>    return Length;
>  }
> @@ -571,8 +575,12 @@ AsciiStrnLenS (
>    // String then AsciiStrnLenS returns MaxSize. At most the first MaxSize
> characters of String shall
>    // be accessed by AsciiStrnLenS.
>    //
> -  for (Length = 0; (Length < MaxSize) && (*String != 0); String++, Length++) {
> -    ;
> +  Length = 0;
> +  while (String[Length] != 0) {
> +    if (Length >= MaxSize - 1) {
> +      return MaxSize;
> +    }
> +    Length++;
>    }
>    return Length;
>  }
> --
> 1.9.5.msysgit.0

[PATCH v2 2/6] MdePkg/BaseLib: Add an additional check within (Ascii)StrnCmp

[Hao Wu]

This commit adds an addtional check in AsciiStrnCmp and StrnCmp. It
explicitly checks the end of the sting pointed by 'SecondString' to make
the code logic easier for reading and to prevent possible mis-reports by
static code checkers.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
---
 MdePkg/Library/BaseLib/String.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index 25962f8..fa96d1c 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -1,7 +1,7 @@
 /** @file
   Unicode and ASCII string primitives.
 
-  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<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
@@ -315,6 +315,7 @@ StrnCmp (
   }
 
   while ((*FirstString != L'\0') &&
+         (*SecondString != L'\0') &&
          (*FirstString == *SecondString) &&
          (Length > 1)) {
     FirstString++;
@@ -1474,6 +1475,7 @@ AsciiStrnCmp (
   }
 
   while ((*FirstString != '\0') &&
+         (*SecondString != '\0') &&
          (*FirstString == *SecondString) &&
          (Length > 1)) {
     FirstString++;
-- 
1.9.5.msysgit.0

[PATCH v2 3/6] MdePkg/MemoryLib: Refine InternalMemSetMem16|32|64 functions logic

[Hao Wu]

This commit refines the logic for InternalMemSetMem16|32|64 functions. It
avoids using the decrement operator '--' for array index to prevent
possible mis-reports by static code checkers.

Please note that those modified functions are only consumed within
MemoryLib by APIs SetMem16|32|64, and those APIs will handle the case when
the input number of bytes to set is 0. Hence, the behavior of APIs
SetMem16|32|64 is not changed.

Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
---
 MdePkg/Library/BaseMemoryLib/MemLibGeneric.c | 18 +++++++++---------
 MdePkg/Library/PeiMemoryLib/MemLibGeneric.c  | 18 +++++++++---------
 MdePkg/Library/UefiMemoryLib/MemLibGeneric.c | 18 +++++++++---------
 3 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/MdePkg/Library/BaseMemoryLib/MemLibGeneric.c b/MdePkg/Library/BaseMemoryLib/MemLibGeneric.c
index b058be8..cf40ace 100644
--- a/MdePkg/Library/BaseMemoryLib/MemLibGeneric.c
+++ b/MdePkg/Library/BaseMemoryLib/MemLibGeneric.c
@@ -37,9 +37,9 @@ InternalMemSetMem16 (
   IN      UINT16                    Value
   )
 {
-  do {
-    ((UINT16*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT16*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
@@ -61,9 +61,9 @@ InternalMemSetMem32 (
   IN      UINT32                    Value
   )
 {
-  do {
-    ((UINT32*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT32*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
@@ -85,9 +85,9 @@ InternalMemSetMem64 (
   IN      UINT64                    Value
   )
 {
-  do {
-    ((UINT64*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT64*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
diff --git a/MdePkg/Library/PeiMemoryLib/MemLibGeneric.c b/MdePkg/Library/PeiMemoryLib/MemLibGeneric.c
index 490b244..ed18b57 100644
--- a/MdePkg/Library/PeiMemoryLib/MemLibGeneric.c
+++ b/MdePkg/Library/PeiMemoryLib/MemLibGeneric.c
@@ -37,9 +37,9 @@ InternalMemSetMem16 (
   IN      UINT16                    Value
   )
 {
-  do {
-    ((UINT16*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT16*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
@@ -61,9 +61,9 @@ InternalMemSetMem32 (
   IN      UINT32                    Value
   )
 {
-  do {
-    ((UINT32*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT32*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
@@ -85,9 +85,9 @@ InternalMemSetMem64 (
   IN      UINT64                    Value
   )
 {
-  do {
-    ((UINT64*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT64*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
diff --git a/MdePkg/Library/UefiMemoryLib/MemLibGeneric.c b/MdePkg/Library/UefiMemoryLib/MemLibGeneric.c
index da02b6c..f1efdbb 100644
--- a/MdePkg/Library/UefiMemoryLib/MemLibGeneric.c
+++ b/MdePkg/Library/UefiMemoryLib/MemLibGeneric.c
@@ -37,9 +37,9 @@ InternalMemSetMem16 (
   IN      UINT16                    Value
   )
 {
-  do {
-    ((UINT16*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT16*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
@@ -61,9 +61,9 @@ InternalMemSetMem32 (
   IN      UINT32                    Value
   )
 {
-  do {
-    ((UINT32*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT32*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
@@ -85,9 +85,9 @@ InternalMemSetMem64 (
   IN      UINT64                    Value
   )
 {
-  do {
-    ((UINT64*)Buffer)[--Length] = Value;
-  } while (Length != 0);
+  for (; Length != 0; Length--) {
+    ((UINT64*)Buffer)[Length - 1] = Value;
+  }
   return Buffer;
 }
 
-- 
1.9.5.msysgit.0

[PATCH v2 4/6] MdeModulePkg/DxeNetLib: Rewrite NetblockChecksum function logic

[Hao Wu]

This commit rewrites the logic for NetblockChecksum. It processes the
checksum of the left-over byte first to prevent possible mis-reports by
static code checkers.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 MdeModulePkg/Library/DxeNetLib/NetBuffer.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/MdeModulePkg/Library/DxeNetLib/NetBuffer.c b/MdeModulePkg/Library/DxeNetLib/NetBuffer.c
index bbbdbc0..95cb717 100644
--- a/MdeModulePkg/Library/DxeNetLib/NetBuffer.c
+++ b/MdeModulePkg/Library/DxeNetLib/NetBuffer.c
@@ -1,7 +1,7 @@
 /** @file
   Network library functions providing net buffer operation support.
 
-Copyright (c) 2005 - 2010, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.<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
@@ -1661,6 +1661,13 @@ NetblockChecksum (
 
   Sum = 0;
 
+  //
+  // Add left-over byte, if any
+  //
+  if (Len % 2 != 0) {
+    Sum += *(Bulk + Len - 1);
+  }
+
   while (Len > 1) {
     Sum += *(UINT16 *) Bulk;
     Bulk += 2;
@@ -1668,13 +1675,6 @@ NetblockChecksum (
   }
 
   //
-  // Add left-over byte, if any
-  //
-  if (Len > 0) {
-    Sum += *(UINT8 *) Bulk;
-  }
-
-  //
   // Fold 32-bit sum to 16 bits
   //
   while ((Sum >> 16) != 0) {
-- 
1.9.5.msysgit.0

[PATCH v2 5/6] MdeModulePkg/UefiPxeBcDxe: Refine the CvtNum function logic

[Hao Wu]

This commit refines the logic for the CvtNum function. It avoids using the
decrement operator '--' for array index to prevent possible mis-reports by
static code checkers.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcSupport.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcSupport.c b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcSupport.c
index 0865ddd..0779056 100644
--- a/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcSupport.c
+++ b/MdeModulePkg/Universal/Network/UefiPxeBcDxe/PxeBcSupport.c
@@ -132,11 +132,10 @@ CvtNum (
 {
   UINTN Remainder;
 
-  while (Length > 0) {
+  for (; Length > 0; Length--) {
     Remainder = Number % 10;
     Number /= 10;
-    Length--;
-    Buffer[Length] = (UINT8) ('0' + Remainder);
+    Buffer[Length - 1] = (UINT8) ('0' + Remainder);
   }
 }
 
-- 
1.9.5.msysgit.0

[PATCH v2 6/6] NetworkPkg: Refine UintnToAscDecWithFormat functions logic

[Hao Wu]

This commit refines the logic for HttpBootUintnToAscDecWithFormat and
PxeBcUintnToAscDecWithFormat. It avoids using the decrement operator '--'
for array index to prevent possible mis-reports by static code checkers.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/HttpBootDxe/HttpBootSupport.c | 5 ++---
 NetworkPkg/UefiPxeBcDxe/PxeBcSupport.c   | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/NetworkPkg/HttpBootDxe/HttpBootSupport.c b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
index 9410bf9..bdb29ae 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootSupport.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootSupport.c
@@ -86,11 +86,10 @@ HttpBootUintnToAscDecWithFormat (
 {
   UINTN                          Remainder;
 
-  while (Length > 0) {
-    Length--;
+  for (; Length > 0; Length--) {
     Remainder      = Number % 10;
     Number        /= 10;
-    Buffer[Length] = (UINT8) ('0' + Remainder);
+    Buffer[Length - 1] = (UINT8) ('0' + Remainder);
   }
 }
 
diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcSupport.c b/NetworkPkg/UefiPxeBcDxe/PxeBcSupport.c
index 00c652d..568360d 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcSupport.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcSupport.c
@@ -1383,11 +1383,10 @@ PxeBcUintnToAscDecWithFormat (
 {
   UINTN                          Remainder;
 
-  while (Length > 0) {
-    Length--;
+  for (; Length > 0; Length--) {
     Remainder      = Number % 10;
     Number        /= 10;
-    Buffer[Length] = (UINT8) ('0' + Remainder);
+    Buffer[Length - 1] = (UINT8) ('0' + Remainder);
   }
 }
 
-- 
1.9.5.msysgit.0