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.zh...@intel.com> > Cc: Wang, Jian J <jian.j.w...@intel.com>; Lu, XiaoyuX <xiaoyux...@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.w...@intel.com> > > Cc: Xiaoyu Lu <xiaoyux...@intel.com> > > Signed-off-by: Shenglei Zhang <shenglei.zh...@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 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#52198): https://edk2.groups.io/g/devel/message/52198 Mute This Topic: https://groups.io/mt/66944007/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
