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 9269721A16EFD for ; Fri, 19 May 2017 01:16:17 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 093307AE8B; Fri, 19 May 2017 08:16:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 093307AE8B Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 093307AE8B 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 112C2189EF; Fri, 19 May 2017 08:16:15 +0000 (UTC) To: Sergei Temerkhanov Cc: "Gao, Liming" , "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> From: Laszlo Ersek Message-ID: Date: Fri, 19 May 2017 10:16:15 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 19 May 2017 08:16:17 +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:16:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 05/19/17 04:45, Sergei Temerkhanov wrote: > On Thu, May 18, 2017 at 1:19 PM, Laszlo Ersek wrote: >> 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. > > On 32-bit architectures, using 64-bit types here may change the ABI. Which might > affect some corner cases like linking precompiled object files to the > library in question. True. I missed the fact that in edk2 you can have binary-only library instances. I should have remembered, after all I had filed :) So yes, UINTN is the best choice; it keeps binary compat beyond everything else. Thanks! Laszlo > >> 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 >>> >>