[PATCH EDK2 v1 0/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check

[PATCH EDK2 v1 0/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check

[wenyi,xie]

Main Changes :
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.

Wenyi Xie (1):
  MdeModulePkg/FileExplorerLib: remove redundant null pointer check

 MdeModulePkg/Library/FileExplorerLib/FileExplorer.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

-- 
2.20.1.windows.1

[PATCH EDK2 v1 1/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check

[wenyi,xie]

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.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
---
 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);
-- 
2.20.1.windows.1

回复: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check

[gaoliming]

Agree this change. Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>

This patch can be merged after the stable tag 202011. 

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+67917+4905953+8761045@groups.io
> <bounce+27952+67917+4905953+8761045@groups.io> 代表 wenyi,xie via
> groups.io
> 发送时间: 2020年11月25日 9:53
> 收件人: devel@edk2.groups.io; jian.j.wang@intel.com; hao.a.wu@intel.com;
> dandan.bi@intel.com; eric.dong@intel.com
> 抄送: songdongkuang@huawei.com; xiewenyi2@huawei.com
> 主题: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/FileExplorerLib:
> remove redundant null pointer check
> 
> 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.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
> ---
>  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);
> --
> 2.20.1.windows.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check

[Laszlo Ersek]

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 <stddef.h> (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 <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
> ---
>  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);
> 

Re: [edk2-devel] [PATCH EDK2 v1 1/1] MdeModulePkg/FileExplorerLib: remove redundant null pointer check

[wenyi,xie]

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 <stddef.h> (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 <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Dandan Bi <dandan.bi@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Signed-off-by: Wenyi Xie <xiewenyi2@huawei.com>
>> ---
>>  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);
>>
> 
> .
>