From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5CA0621A16EFD for ; Fri, 19 May 2017 01:12:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B7F803B73F; Fri, 19 May 2017 08:12:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com B7F803B73F Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com B7F803B73F Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-108.phx2.redhat.com [10.3.116.108]) by smtp.corp.redhat.com (Postfix) with ESMTP id A82627E5D8; Fri, 19 May 2017 08:12:24 +0000 (UTC) To: "Gao, Liming" , Sergei Temerkhanov Cc: "edk2-devel@lists.01.org" References: <1494903391-716-1-git-send-email-s.temerkhanov@gmail.com> <1494903391-716-2-git-send-email-s.temerkhanov@gmail.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D72E8BC@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14D730AC8@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <4e6475b6-8af3-3b7a-e086-d5fffcb3dddd@redhat.com> Date: Fri, 19 May 2017 10:12:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14D730AC8@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 19 May 2017 08:12:25 +0000 (UTC) Subject: Re: [PATCH] MdePkg: Fix undefined behavior on variadic parameters X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 May 2017 08:12:26 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 05/19/17 03:35, Gao, Liming wrote: > Laszlo: > Based on your analysis, the parameter type should be changed from BOOLEAN to INT32 to match its promotion type. Right? Not necessarily. BOOLEAN (unsigned char) is indeed promoted to INT32 (int), so BOOLEAN is not appropriate. But any integer type that is compatible with the promoted type is fine. Such types are basically all the integer types whose integer conversion rank is equal to, or larger than, the rank of "int" and "unsigned int". That means INT32, UINT32, INT64, UINT64, INTN, UINTN. Any of these are fine, because they are not changed by integer promotion. The patch picked UINTN, so it's OK. Laszlo > > Thanks > Liming >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Laszlo Ersek >> Sent: Thursday, May 18, 2017 6:20 PM >> To: Sergei Temerkhanov ; Gao, Liming >> >> Cc: edk2-devel@lists.01.org >> Subject: Re: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic >> parameters >> >> On 05/16/17 14:10, Sergei Temerkhanov wrote: >>> On Tue, May 16, 2017 at 8:10 AM, Gao, Liming >> wrote: >>>> Sergey: >>>> Could you give more detail on the undefined behavior on variadic >> parameters? >>>> >>>> I see https://bugzilla.tianocore.org/show_bug.cgi?id=410 describe this >> issues found in the latest CLANG tool chain. Do you find other tool chain >> reports it? >>> >>> Yes, this is exactly the bug this patch fixes. >>> >>> As per the C99 standard: >>> "The parameter parmN is the identifier of the rightmost parameter in >>> the variable parameter list in the function definition (the one just >>> before the , ...). If the parameter parmN is declared with the >>> register storage class, with a function or array type, or with a type >>> that is not compatible with the type that results after application of >>> the default argument promotions, the behavior is undefined." >>> >>> That's exactly the case here since BOOLEAN is a typedef for unsigned >>> char. It undergoes a promotion to an unsigned int >> >> Side topic: >> >> It is promoted, but not to "unsigned int". >> >> The standard says, in "6.3.1.1 Boolean, characters, and integers", >> paragraph 2, >> >> The following may be used in an expression wherever an /int/ or >> /unsigned int/ may be used: >> >> — An object or expression with an integer type whose integer >> conversion rank is less than or equal to the rank of /int/ and >> /unsigned int/. >> — A bit-field of type /_Bool/, /int/, /signed int/, or >> /unsigned int/. >> >> If an /int/ can represent all values of the original type, the value >> is converted to an /int/; otherwise, it is converted to an >> /unsigned int/. These are called the /integer promotions/. [...] >> >> On all supported edk2 platforms, "unsigned char"'s range is 0..255 >> inclusive, which can be represented by "int" (again on all supported >> edk2 platforms). So the promotion occurs to "int", not "unsigned int" >> >> >> Furthermore, in place of the suggested UINTN type (which is fine), the >> following further types would be correct: INT32, UINT32, INT64, UINT64, >> INTN. The reason is that all of these map to standard C types, on all >> edk2 platforms, whose integer conversion ranks are not less than that of >> "int" and "unsigned int". Hence they are all unaffected by the integer >> promotions. >> >> (This digression does not affect your main point, which remains correct; >> I just wanted to be precise here, since we're quoting the standard.) >> >> Thanks >> Laszlo >> >>> which is not a >>> compatible type for unsigned char. Correct me if I'm wrong. >>> >>> Regards, >>> Sergey >>> >>>> >>>> Thanks >>>> Liming >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Sergey Temerkhanov >>>>> Sent: Tuesday, May 16, 2017 10:57 AM >>>>> To: edk2-devel@lists.01.org >>>>> Subject: [edk2] [PATCH] MdePkg: Fix undefined behavior on variadic >> parameters >>>>> >>>>> Fix undefined behavior by avoiding parameter type promotion >>>>> >>>>> Signed-off-by: Sergey Temerkhanov >>>>> --- >>>>> MdePkg/Include/Library/UefiLib.h | 2 +- >>>>> MdePkg/Library/UefiLib/UefiLib.c | 2 +- >>>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/MdePkg/Include/Library/UefiLib.h >> b/MdePkg/Include/Library/UefiLib.h >>>>> index 0b14792..4e4697c 100644 >>>>> --- a/MdePkg/Include/Library/UefiLib.h >>>>> +++ b/MdePkg/Include/Library/UefiLib.h >>>>> @@ -818,7 +818,7 @@ CHAR8 * >>>>> EFIAPI >>>>> GetBestLanguage ( >>>>> IN CONST CHAR8 *SupportedLanguages, >>>>> - IN BOOLEAN Iso639Language, >>>>> + IN UINTN Iso639Language, >>>>> ... >>>>> ); >>>>> >>>>> diff --git a/MdePkg/Library/UefiLib/UefiLib.c >> b/MdePkg/Library/UefiLib/UefiLib.c >>>>> index a7eee01..74528ec 100644 >>>>> --- a/MdePkg/Library/UefiLib/UefiLib.c >>>>> +++ b/MdePkg/Library/UefiLib/UefiLib.c >>>>> @@ -1514,7 +1514,7 @@ CHAR8 * >>>>> EFIAPI >>>>> GetBestLanguage ( >>>>> IN CONST CHAR8 *SupportedLanguages, >>>>> - IN BOOLEAN Iso639Language, >>>>> + IN UINTN Iso639Language, >>>>> ... >>>>> ) >>>>> { >>>>> -- >>>>> 2.7.4 >>>>> >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> edk2-devel@lists.01.org >>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel