Re: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from 4 to 8-byte size

["Zhang, Shenglei" <shenglei.zhang@intel.com>]

Thanks, Lazslo. I will add it to the exception list on my local tool.

Thanks,
Shenglei

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Saturday, December 7, 2019 10:11 PM
> To: devel@edk2.groups.io; Zhang, Shenglei <shenglei.zhang@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: Re: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from 4
> to 8-byte size
> 
> On 12/05/19 09:46, Zhang, Shenglei wrote:
> > tp, pch, digits and xdigits are both 4-byte-size, but not
> > cast to 8-byte-size when operated with 8-byte-size variables.
> > This is a issue reported by my local static tool.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
> > Signed-off-by: Shenglei Zhang <shenglei.zhang@intel.com>
> > ---
> >  CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> > index 32e1ab8690e6..ad392b18ca66 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> > @@ -132,7 +132,7 @@ inet_pton4(
> >  		const char *pch;
> >
> >  		if ((pch = strchr(digits, ch)) != NULL) {
> > -			u_int new = *tp * 10 + (u_int)(pch - digits);
> > 			u_int new = (u_int)(*tp) * 10 + (u_int)pch -
> (u_int)digits;
> >
> >  			if (new > 255)
> >  				return (0);
> > @@ -200,7 +200,7 @@ inet_pton6(
> >  			pch = strchr((xdigits = xdigits_u), ch);
> >  		if (pch != NULL) {
> >  			val <<= 4;
> > -			val |= (pch - xdigits);
> > 			val |= (u_int)pch - (u_int)xdigits;
> >  			if (val > 0xffff)
> >  				return (0);
> >  			saw_xdigit = 1;
> >
> 
> (1) This email does not look like a real patch for edk2.
> 
> It removes some lines, yes, but the expressions that (I think?) it
> proposes, as new lines, are not marked with "+". Instead, those are
> displayed as existent code ("context").
> 
> But the file does not contain lines such as
> 
> u_int new = (u_int)(*tp) * 10 + (u_int)pch - (u_int)digits;
> 
> and
> 
> val |= (u_int)pch - (u_int)xdigits;
> 
> I don't understand how this patch was generated. Maybe you added the
> new
> lines in a separate patch before, and removed the old lines in a new
> patch, and posted only the last (= partial change) patch.
> 
> 
> (2) We can spell out the current edk2 types in the definition (and
> initialization) of "new" below
> 
>   u_int new = *tp * 10 + (u_int)(pch - digits);
> 
> as follows:
> 
>   UINTN new = (UINT8)*tp * (INT32)10 +
>               (UINTN)((CONST CHAR8 *)pch - (CONST CHAR8 *)digits);
> 
> I don't have the slightest idea why a static analyzer whines about this.
> 
> - the subtraction of the pointers is valid ("pch" points into "digits"),
> - the result of the subtraction is a ptrdiff_t,
> - ptrdiff_t can be safely converted to UINTN.
> 
> Furthermore,
> 
> - In the multiplication, UINT8 is promoted to INT32, and the value is
> non-negative, and does not exceed 255
> - the multiplication is performed in INT32, and could never overflow
> (because 255 * 10 = 2550 is representable in INT32),
> - UINTN is either UINT32 or UINT64; for the addition, the INT32 product
> is converted to UINTN,
> - the addition is performed in UINTN,
> - the result is stored to a UINTN.
> 
> I think the static analyzer warning is wrong.
> 
> I'd rather we supressed any such warning in some other way, for example
> in the configuration of your static analyzer. Unless we find a critical
> bug or evidently undefined behavior in this code, I'd like to keep it
> intact (matching its origin from edk2-libc).
> 
> 
> (3) In the assignment expression statement
> 
>   val |= (pch - xdigits);
> 
> the subtraction uses (CONST CHAR8 *) operands. It is a valid subtraction
> ("pch" points into the array pointed-to by "xdigits"). The result is of
> type "ptrdiff_t" (per C spec), and has non-negative value.
> 
> "val" is UINTN.
> 
> Therefore we can spell out the above compound assignment as the below
> simple assignment:
> 
>   val = (UINTN)val | (ptrdiff_t)(pch - xdigits);
> 
> whicn means, in practice:
> 
> - on 32-bit:
> 
>   val = (unsigned)val | (long)(pch - xdigits);
> 
> - on 64-bit:
> 
>   val = (unsigned long long)val | (long)(pch - xdigits);
> 
> In the 64-bit case, the (long) difference is converted to (unsigned long
> long), per usual arithmetic conversions.
> 
> In the 32-bit case, both operands are converted to (unsigned long)
> (because "long", on our platforms, cannot represent all values of
> "unsigned int"). The result, of type "unsigned long", is assigned to
> "val", of type "unsigned int". However, this cannot cause a loss of
> information, because all values of the non-negative (long) difference
> fit in "unsigned int".
> 
> So I don't think this code needs to be changed either. (Although I agree
> it's not too easy to reason about.)
> 
> 
> (4) Even if the patch was well-formed, and even if I agreed with
> modifying these expressions to something else, replacing the pointer
> subtractions with integer subtractions, as in:
> 
>   (u_int)pch - (u_int)digits
> 
> and
> 
>   (u_int)pch - (u_int)xdigits
> 
> makes things actually *harder* to understand. Semantically, we don't
> want to calculate the difference between the numerical representations
> of these pointers; instead, we want the offsets of the found elements
> into the containing arrays.
> 
> --*--
> 
> I think I could agree to one change:
> 
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c
> @@ -200,7 +200,7 @@ inet_pton6(
>                         pch = strchr((xdigits = xdigits_u), ch);
>                 if (pch != NULL) {
>                         val <<= 4;
> -                       val |= (pch - xdigits);
> +                       val |= (u_int)(pch - xdigits);
>                         if (val > 0xffff)
>                                 return (0);
>                         saw_xdigit = 1;
> 
> For the following reasons:
> 
> - it would be stylistically consistent with the rest of this file;
> 
> - it would make the pointer subtraction in inet_pton6() consistent with
> the pointer subtraction in inet_pton4(), where we have
> 
>   (u_int)(pch - digits)
> 
> already;
> 
> - although not technically necessary, this change would significantly
> simplify the reasoning about the expression.
> 
> Thanks
> Laszlo
> 
> 
>