From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.1771.1575727886070300517 for ; Sat, 07 Dec 2019 06:11:26 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MU0YaEBg; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575727885; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y0VwXc2UmOEoKhMsPlQBsaplrXaKiMq6wFwAkLGUj2c=; b=MU0YaEBgX79COxAWuCgEQcl+Bco+zx0o+uAqkaYwDDQh7/2BXXJ9BmWcyiMEztrSw04d/x g1v4j6MfiLYInrdiBjXxGjmXbiCyMk7/0+nfipgJOMzHcUvLtKDlJIASgymYnqaFwlB6tZ 3vYFmNCK0js/nRnx0Tg0Jmh36KUVnMA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-381-5-lpDKJCPdq9UIqrVahnhQ-1; Sat, 07 Dec 2019 09:11:21 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 629F71005502; Sat, 7 Dec 2019 14:11:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-34.ams2.redhat.com [10.36.116.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2B9145D6BE; Sat, 7 Dec 2019 14:11:18 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] CryptoPkg/SysCall: Cast variables from 4 to 8-byte size To: devel@edk2.groups.io, shenglei.zhang@intel.com Cc: Jian J Wang , Xiaoyu Lu References: <20191205084607.34208-1-shenglei.zhang@intel.com> From: "Laszlo Ersek" Message-ID: <0ced9d3a-5cba-becc-cf9c-91da53c028f7@redhat.com> Date: Sat, 7 Dec 2019 15:11:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191205084607.34208-1-shenglei.zhang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: 5-lpDKJCPdq9UIqrVahnhQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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( > 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