From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mx.groups.io with SMTP id smtpd.web09.7690.1576217520316651988 for ; Thu, 12 Dec 2019 22:12:00 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.151, mailfrom: shenglei.zhang@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Dec 2019 22:12:00 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,308,1571727600"; d="scan'208";a="239199219" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga004.fm.intel.com with ESMTP; 12 Dec 2019 22:12:00 -0800 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 12 Dec 2019 22:11:59 -0800 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Thu, 12 Dec 2019 22:11:59 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Thu, 12 Dec 2019 22:11:59 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.71]) with mapi id 14.03.0439.000; Fri, 13 Dec 2019 14:11:57 +0800 From: "Zhang, Shenglei" To: "devel@edk2.groups.io" , "lersek@redhat.com" CC: "Wang, Jian J" , "Lu, XiaoyuX" Subject: Re: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from 4 to 8-byte size Thread-Topic: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from 4 to 8-byte size Thread-Index: AQHVrQgxMkg9jwCOTUaaOLuLX19RYKe3ndKw Date: Fri, 13 Dec 2019 06:11:57 +0000 Message-ID: References: <20191205084607.34208-1-shenglei.zhang@intel.com> <0ced9d3a-5cba-becc-cf9c-91da53c028f7@redhat.com> In-Reply-To: <0ced9d3a-5cba-becc-cf9c-91da53c028f7@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: shenglei.zhang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 > Cc: Wang, Jian J ; Lu, XiaoyuX > Subject: Re: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from= 4 > to 8-byte size >=20 > 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 > > Cc: Xiaoyu Lu > > Signed-off-by: Shenglei Zhang > > --- > > 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( > > =09const char *pch; > > > > =09if ((pch =3D strchr(digits, ch)) !=3D NULL) { > > - =09 u_int new =3D *tp * 10 + (u_int)(pch - digits); > > =09 =09u_int new =3D (u_int)(*tp) * 10 + (u_int)pch - > (u_int)digits; > > > > =09 if (new > 255) > > =09 =09return (0); > > @@ -200,7 +200,7 @@ inet_pton6( > > =09 pch =3D strchr((xdigits =3D xdigits_u), ch); > > =09if (pch !=3D NULL) { > > =09 val <<=3D 4; > > - =09 val |=3D (pch - xdigits); > > =09 =09val |=3D (u_int)pch - (u_int)xdigits; > > =09 if (val > 0xffff) > > =09 =09return (0); > > =09 saw_xdigit =3D 1; > > >=20 > (1) This email does not look like a real patch for edk2. >=20 > 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"). >=20 > But the file does not contain lines such as >=20 > u_int new =3D (u_int)(*tp) * 10 + (u_int)pch - (u_int)digits; >=20 > and >=20 > val |=3D (u_int)pch - (u_int)xdigits; >=20 > 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 (=3D partial change) patch. >=20 >=20 > (2) We can spell out the current edk2 types in the definition (and > initialization) of "new" below >=20 > u_int new =3D *tp * 10 + (u_int)(pch - digits); >=20 > as follows: >=20 > UINTN new =3D (UINT8)*tp * (INT32)10 + > (UINTN)((CONST CHAR8 *)pch - (CONST CHAR8 *)digits); >=20 > I don't have the slightest idea why a static analyzer whines about this. >=20 > - 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. >=20 > Furthermore, >=20 > - 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 =3D 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. >=20 > I think the static analyzer warning is wrong. >=20 > 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). >=20 >=20 > (3) In the assignment expression statement >=20 > val |=3D (pch - xdigits); >=20 > 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. >=20 > "val" is UINTN. >=20 > Therefore we can spell out the above compound assignment as the below > simple assignment: >=20 > val =3D (UINTN)val | (ptrdiff_t)(pch - xdigits); >=20 > whicn means, in practice: >=20 > - on 32-bit: >=20 > val =3D (unsigned)val | (long)(pch - xdigits); >=20 > - on 64-bit: >=20 > val =3D (unsigned long long)val | (long)(pch - xdigits); >=20 > In the 64-bit case, the (long) difference is converted to (unsigned long > long), per usual arithmetic conversions. >=20 > 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". >=20 > So I don't think this code needs to be changed either. (Although I agree > it's not too easy to reason about.) >=20 >=20 > (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: >=20 > (u_int)pch - (u_int)digits >=20 > and >=20 > (u_int)pch - (u_int)xdigits >=20 > 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. >=20 > --*-- >=20 > I think I could agree to one change: >=20 > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/inet_pton.c > @@ -200,7 +200,7 @@ inet_pton6( > pch =3D strchr((xdigits =3D xdigits_u), ch); > if (pch !=3D NULL) { > val <<=3D 4; > - val |=3D (pch - xdigits); > + val |=3D (u_int)(pch - xdigits); > if (val > 0xffff) > return (0); > saw_xdigit =3D 1; >=20 > For the following reasons: >=20 > - it would be stylistically consistent with the rest of this file; >=20 > - it would make the pointer subtraction in inet_pton6() consistent with > the pointer subtraction in inet_pton4(), where we have >=20 > (u_int)(pch - digits) >=20 > already; >=20 > - although not technically necessary, this change would significantly > simplify the reasoning about the expression. >=20 > Thanks > Laszlo >=20 >=20 >=20