Re: [PATCH] MdePkg/BaseLib: Enhance the return value for string to uint functions

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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


> -----Original Message-----
> From: Wu, Hao A
> Sent: Thursday, December 8, 2016 2:08 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Wu, Jiaxin <jiaxin.wu@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH] MdePkg/BaseLib: Enhance the return value for string to
> uint functions
> 
> For the following APIs in MdePkg/BaseLib:
> AsciiStrDecimalToUintn
> AsciiStrDecimalToUint64
> AsciiStrHexToUintn
> AsciiStrHexToUint64
> StrDecimalToUintn
> StrDecimalToUint64
> StrHexToUintn
> StrHexToUint64
> 
> They will ASSERT for DEBUG build when the input string exceeds the range of
> UINTN/UINT64. However, for RELEASE build, incorrect value will be returned.
> 
> This commit removes those exceed-range ASSERT checks and makes those
> APIs to return MAX_UINTN/MAX_UINT64 instead.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@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/Include/Library/BaseLib.h | 20 ++++++------
> MdePkg/Library/BaseLib/String.c  | 70 ++++++++++++++++++++++++--------
> --------
>  2 files changed, 53 insertions(+), 37 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/BaseLib.h
> b/MdePkg/Include/Library/BaseLib.h
> index b69c703..d89d1ec 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -879,7 +879,7 @@ StrStr (
>    If String has no pad spaces or valid decimal digits,
>    then 0 is returned.
>    If the number represented by String overflows according
> -  to the range defined by UINTN, then ASSERT().
> +  to the range defined by UINTN, then MAX_UINTN is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains
>    more than PcdMaximumUnicodeStringLength Unicode characters not
> including @@ -919,7 +919,7 @@ StrDecimalToUintn (
>    If String has no pad spaces or valid decimal digits,
>    then 0 is returned.
>    If the number represented by String overflows according
> -  to the range defined by UINT64, then ASSERT().
> +  to the range defined by UINT64, then MAX_UINT64 is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains
>    more than PcdMaximumUnicodeStringLength Unicode characters not
> including @@ -961,7 +961,7 @@ StrDecimalToUint64 (
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
>    then zero is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINTN, then ASSERT().
> +  UINTN, then MAX_UINTN is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains more
> than
>    PcdMaximumUnicodeStringLength Unicode characters not including the
> Null-terminator, @@ -1003,7 +1003,7 @@ StrHexToUintn (
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
>    then zero is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINT64, then ASSERT().
> +  UINT64, then MAX_UINT64 is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains more
> than
>    PcdMaximumUnicodeStringLength Unicode characters not including the
> Null-terminator, @@ -1481,7 +1481,7 @@ AsciiStrStr (
>    If String has only pad spaces, then 0 is returned.
>    If String has no pad spaces or valid decimal digits, then 0 is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINTN, then ASSERT().
> +  UINTN, then MAX_UINTN is returned.
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero, and String contains more than
>    PcdMaximumAsciiStringLength ASCII characters not including the Null-
> terminator, @@ -1518,7 +1518,7 @@ AsciiStrDecimalToUintn (
>    If String has only pad spaces, then 0 is returned.
>    If String has no pad spaces or valid decimal digits, then 0 is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINT64, then ASSERT().
> +  UINT64, then MAX_UINT64 is returned.
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero, and String contains more than
>    PcdMaximumAsciiStringLength ASCII characters not including the Null-
> terminator, @@ -1557,9 +1557,9 @@ AsciiStrDecimalToUint64 (
>    If String has only pad spaces, then 0 is returned.
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
> then
>    0 is returned.
> -
>    If the number represented by String overflows according to the range
> defined by UINTN,
> -  then ASSERT().
> +  then MAX_UINTN is returned.
> +
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero,
>    and String contains more than PcdMaximumAsciiStringLength ASCII
> characters not including @@ -1598,9 +1598,9 @@ AsciiStrHexToUintn (
>    If String has only pad spaces, then 0 is returned.
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
> then
>    0 is returned.
> -
>    If the number represented by String overflows according to the range
> defined by UINT64,
> -  then ASSERT().
> +  then MAX_UINT64 is returned.
> +
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero,
>    and String contains more than PcdMaximumAsciiStringLength ASCII
> characters not including diff --git a/MdePkg/Library/BaseLib/String.c
> b/MdePkg/Library/BaseLib/String.c index 25962f8..efcfdfe 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 @@ -637,7 +637,7 @@ InternalIsHexaDecimalDigitCharacter (
>    If String has no pad spaces or valid decimal digits,
>    then 0 is returned.
>    If the number represented by String overflows according
> -  to the range defined by UINTN, then ASSERT().
> +  to the range defined by UINTN, then MAX_UINTN is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains
>    more than PcdMaximumUnicodeStringLength Unicode characters, not
> including @@ -681,9 +681,11 @@ StrDecimalToUintn (
>    while (InternalIsDecimalDigitCharacter (*String)) {
>      //
>      // If the number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINTN, then MAX_UINTN is returned.
>      //
> -    ASSERT (Result <= ((((UINTN) ~0) - (*String - L'0')) / 10));
> +    if (Result > ((MAX_UINTN - (*String - L'0')) / 10)) {
> +      return MAX_UINTN;
> +    }
> 
>      Result = Result * 10 + (*String - L'0');
>      String++;
> @@ -716,7 +718,7 @@ StrDecimalToUintn (
>    If String has no pad spaces or valid decimal digits,
>    then 0 is returned.
>    If the number represented by String overflows according
> -  to the range defined by UINT64, then ASSERT().
> +  to the range defined by UINT64, then MAX_UINT64 is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains
>    more than PcdMaximumUnicodeStringLength Unicode characters, not
> including @@ -760,9 +762,11 @@ StrDecimalToUint64 (
>    while (InternalIsDecimalDigitCharacter (*String)) {
>      //
>      // If the number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINT64, then MAX_UINT64 is returned.
>      //
> -    ASSERT (Result <= DivU64x32 (((UINT64) ~0) - (*String - L'0') , 10));
> +    if (Result > DivU64x32 (MAX_UINT64 - (*String - L'0'), 10)) {
> +      return MAX_UINT64;
> +    }
> 
>      Result = MultU64x32 (Result, 10) + (*String - L'0');
>      String++;
> @@ -795,7 +799,7 @@ StrDecimalToUint64 (
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
>    then zero is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINTN, then ASSERT().
> +  UINTN, then MAX_UINTN is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains more
> than
>    PcdMaximumUnicodeStringLength Unicode characters, not including the
> Null-terminator, @@ -849,9 +853,11 @@ StrHexToUintn (
>    while (InternalIsHexaDecimalDigitCharacter (*String)) {
>      //
>      // If the Hex Number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINTN, then MAX_UINTN is returned.
>      //
> -    ASSERT (Result <= ((((UINTN) ~0) - InternalHexCharToUintn (*String)) >>
> 4));
> +    if (Result > ((MAX_UINTN - InternalHexCharToUintn (*String)) >> 4)) {
> +      return MAX_UINTN;
> +    }
> 
>      Result = (Result << 4) + InternalHexCharToUintn (*String);
>      String++;
> @@ -885,7 +891,7 @@ StrHexToUintn (
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
>    then zero is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINT64, then ASSERT().
> +  UINT64, then MAX_UINT64 is returned.
> 
>    If PcdMaximumUnicodeStringLength is not zero, and String contains more
> than
>    PcdMaximumUnicodeStringLength Unicode characters, not including the
> Null-terminator, @@ -940,9 +946,11 @@ StrHexToUint64 (
>    while (InternalIsHexaDecimalDigitCharacter (*String)) {
>      //
>      // If the Hex Number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINT64, then MAX_UINT64 is returned.
>      //
> -    ASSERT (Result <= RShiftU64 (((UINT64) ~0) - InternalHexCharToUintn
> (*String) , 4));
> +    if (Result > RShiftU64 (MAX_UINT64 - InternalHexCharToUintn (*String),
> 4)) {
> +      return MAX_UINT64;
> +    }
> 
>      Result = LShiftU64 (Result, 4);
>      Result = Result + InternalHexCharToUintn (*String); @@ -1679,7 +1687,7
> @@ AsciiStrStr (
>    If String has only pad spaces, then 0 is returned.
>    If String has no pad spaces or valid decimal digits, then 0 is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINTN, then ASSERT().
> +  UINTN, then MAX_UINTN is returned.
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero, and String contains more than
>    PcdMaximumAsciiStringLength ASCII characters not including the Null-
> terminator, @@ -1722,9 +1730,11 @@ AsciiStrDecimalToUintn (
>    while (InternalAsciiIsDecimalDigitCharacter (*String)) {
>      //
>      // If the number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINTN, then MAX_UINTN is returned.
>      //
> -    ASSERT (Result <= ((((UINTN) ~0) - (*String - L'0')) / 10));
> +    if (Result > ((MAX_UINTN - (*String - '0')) / 10)) {
> +      return MAX_UINTN;
> +    }
> 
>      Result = Result * 10 + (*String - '0');
>      String++;
> @@ -1753,7 +1763,7 @@ AsciiStrDecimalToUintn (
>    If String has only pad spaces, then 0 is returned.
>    If String has no pad spaces or valid decimal digits, then 0 is returned.
>    If the number represented by String overflows according to the range
> defined by
> -  UINT64, then ASSERT().
> +  UINT64, then MAX_UINT64 is returned.
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero, and String contains more than
>    PcdMaximumAsciiStringLength ASCII characters not including the Null-
> terminator, @@ -1796,9 +1806,11 @@ AsciiStrDecimalToUint64 (
>    while (InternalAsciiIsDecimalDigitCharacter (*String)) {
>      //
>      // If the number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINT64, then MAX_UINT64 is returned.
>      //
> -    ASSERT (Result <= DivU64x32 (((UINT64) ~0) - (*String - L'0') , 10));
> +    if (Result > DivU64x32 (MAX_UINT64 - (*String - '0'), 10)) {
> +      return MAX_UINT64;
> +    }
> 
>      Result = MultU64x32 (Result, 10) + (*String - '0');
>      String++;
> @@ -1828,9 +1840,9 @@ AsciiStrDecimalToUint64 (
>    If String has only pad spaces, then 0 is returned.
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
> then
>    0 is returned.
> -
>    If the number represented by String overflows according to the range
> defined by UINTN,
> -  then ASSERT().
> +  then MAX_UINTN is returned.
> +
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero,
>    and String contains more than PcdMaximumAsciiStringLength ASCII
> characters not including @@ -1884,9 +1896,11 @@ AsciiStrHexToUintn (
>    while (InternalAsciiIsHexaDecimalDigitCharacter (*String)) {
>      //
>      // If the Hex Number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINTN, then MAX_UINTN is returned.
>      //
> -    ASSERT (Result <= ((((UINTN) ~0) - InternalHexCharToUintn (*String)) >>
> 4));
> +    if (Result > ((MAX_UINTN - InternalHexCharToUintn (*String)) >> 4)) {
> +      return MAX_UINTN;
> +    }
> 
>      Result = (Result << 4) + InternalAsciiHexCharToUintn (*String);
>      String++;
> @@ -1917,9 +1931,9 @@ AsciiStrHexToUintn (
>    If String has only pad spaces, then 0 is returned.
>    If String has no leading pad spaces, leading zeros or valid hexadecimal digits,
> then
>    0 is returned.
> -
>    If the number represented by String overflows according to the range
> defined by UINT64,
> -  then ASSERT().
> +  then MAX_UINT64 is returned.
> +
>    If String is NULL, then ASSERT().
>    If PcdMaximumAsciiStringLength is not zero,
>    and String contains more than PcdMaximumAsciiStringLength ASCII
> characters not including @@ -1976,9 +1990,11 @@ AsciiStrHexToUint64 (
>    while (InternalAsciiIsHexaDecimalDigitCharacter (*String)) {
>      //
>      // If the Hex Number represented by String overflows according
> -    // to the range defined by UINTN, then ASSERT().
> +    // to the range defined by UINT64, then MAX_UINT64 is returned.
>      //
> -    ASSERT (Result <= RShiftU64 (((UINT64) ~0) - InternalHexCharToUintn
> (*String) , 4));
> +    if (Result > RShiftU64 (MAX_UINT64 - InternalHexCharToUintn (*String),
> 4)) {
> +      return MAX_UINT64;
> +    }
> 
>      Result = LShiftU64 (Result, 4);
>      Result = Result + InternalAsciiHexCharToUintn (*String);
> --
> 1.9.5.msysgit.0