From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mx.groups.io with SMTP id smtpd.web08.5623.1606354450092043475 for ; Wed, 25 Nov 2020 17:34:11 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.190, mailfrom: xiewenyi2@huawei.com) Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4ChKzD4GYNz15Pj2; Thu, 26 Nov 2020 09:33:44 +0800 (CST) Received: from [10.174.152.217] (10.174.152.217) by DGGEMS411-HUB.china.huawei.com (10.3.19.211) with Microsoft SMTP Server id 14.3.487.0; Thu, 26 Nov 2020 09:33:57 +0800 Subject: Re: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check To: Laszlo Ersek , , , , , CC: References: <1606269171-21979-1-git-send-email-xiewenyi2@huawei.com> <1606269171-21979-2-git-send-email-xiewenyi2@huawei.com> <931ef6c1-8a00-1f0a-b521-bf4e75541290@redhat.com> From: "wenyi,xie" Message-ID: <998734c2-1db2-df65-e0ff-2124bb546d1b@huawei.com> Date: Thu, 26 Nov 2020 09:33:46 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.0.1 MIME-Version: 1.0 In-Reply-To: <931ef6c1-8a00-1f0a-b521-bf4e75541290@redhat.com> X-Originating-IP: [10.174.152.217] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit I see. I will correct commit message soon. Thanks Wenyi On 2020/11/25 23:01, Laszlo Ersek wrote: > On 11/25/20 02:52, wenyi,xie via groups.io wrote: >> Since Info is a pointer of struct EFI_FILE_SYSTEM_VOLUME_LABEL, >> and this struct has only one member VolumeLabel which is char >> array. So Info and Info->VolumeLabel are point to the same >> address, and if Info != NULL, Info->VolumeLabel == NULL is >> always false, it's no necessary to do null pointer check again. > > I agree with the code change, and I agree with Liming that this is > material for *after* the stable tag. > > However, I disagree with the wording of the commit message. I tried to > explain why, on edk2-discuss. Let me try again, here. > > It is *irrelevant* that "Info", and "Info->VolumeLabel", evaluate to the > same pointer values (same addresses). Namely, if "VolumeLabel" were > *not* the first field in EFI_FILE_SYSTEM_VOLUME_LABEL, then this > equality would cease to hold, but the code change would *still* be > correct. Which means that the equality ("same address") is *totally > irrelevant* -- and so it should not be included in the commit message. > > Instead, what matters is that, if "Info" is a *valid* pointer (it points > to a valid EFI_FILE_SYSTEM_VOLUME_LABEL object), then > "Info->VolumeLabel" is a valid *array object*. The expression > "Info->VolumeLabel" has type "array of CHAR16". When evaluating an > expression with array type, the array is implicitly converted to a > pointer to the first element in the array. See the spec quote below: > > ISO C99, > - 6.3 Conversions > - 6.3.2 Other operands > - 6.3.2.1 Lvalues, arrays, and function designators > - paragprah 3: > > Except when it is the operand of the sizeof operator or the unary & > operator, or is a string literal used to initialize an array, an > expression that has type "array of type" is converted to an > expression with type "pointer to type" that points to the initial > element of the array object and is not an lvalue. [...] > > *That* is why (Info != NULL) guarantees that (Info->VolumeLabel != > NULL). Because, we take (Info != NULL) to mean (*Info) is a valid > EFI_FILE_SYSTEM_VOLUME_LABEL object. And so (Info->VolumeLabel) is an > array object, Info->VolumeLabel[0] is a valid CHAR16 object, and the > address of a valid CHAR16 object *cannot be* NULL. For the latter, see > the following spec excerpt: > > ISO C99, > - 6.3 Conversions > - 6.3.2 Other operands > - 6.3.2.3 Pointers > - paragprah 3: > > An integer constant expression with the value 0, or such an > expression cast to type void*, is called a null pointer constant. > [55] If a null pointer constant is converted to a pointer type, the > resulting pointer, called a null pointer, is guaranteed to compare > unequal to a pointer to any object or function. > > Footnote [55]: The macro NULL is defined in (and other > headers) as a null pointer constant; see 7.17. > > > Please replace the commit message with the following: > > """ > If "Info" is a valid pointer to an EFI_FILE_SYSTEM_VOLUME_LABEL > structure, then "Info->VolumeLabel" denotes a valid array object. When > the "Info->VolumeLabel" expression is evaluated, as seen in the > LibFindFileSystem(), it is implicitly converted to > (&Info->VolumeLabel[0]). Because the object described by the expression > (Info->VolumeLabel[0]) is a valid CHAR16 object, its address can never > compare equal to NULL. Therefore, the condition (Info->VolumeLabel == > NULL) will always evaluate to FALSE. Substitute the constant FALSE into > the "if" statement, and simplify the resultant code (eliminate the dead > branch). > """ > > Thanks > Laszlo > >> >> Cc: Jian J Wang >> Cc: Hao A Wu >> Cc: Dandan Bi >> Cc: Eric Dong >> Signed-off-by: Wenyi Xie >> --- >> MdeModulePkg/Library/FileExplorerLib/FileExplorer.c | 10 +++------- >> 1 file changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c >> index 58e49102593c..13a214b06af9 100644 >> --- a/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c >> +++ b/MdeModulePkg/Library/FileExplorerLib/FileExplorer.c >> @@ -821,13 +821,9 @@ LibFindFileSystem ( >> if (Info == NULL) { >> VolumeLabel = L"NO FILE SYSTEM INFO"; >> } else { >> - if (Info->VolumeLabel == NULL) { >> - VolumeLabel = L"NULL VOLUME LABEL"; >> - } else { >> - VolumeLabel = Info->VolumeLabel; >> - if (*VolumeLabel == 0x0000) { >> - VolumeLabel = L"NO VOLUME LABEL"; >> - } >> + VolumeLabel = Info->VolumeLabel; >> + if (*VolumeLabel == 0x0000) { >> + VolumeLabel = L"NO VOLUME LABEL"; >> } >> } >> MenuEntry->DisplayString = AllocateZeroPool (MAX_CHAR); >> > > . >