[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 at intel.com>
> Cc: Hao A Wu <hao.a.wu at intel.com>
> Cc: Dandan Bi <dandan.bi at intel.com>
> Cc: Eric Dong <eric.dong at intel.com>
> Signed-off-by: Wenyi Xie <xiewenyi2 at 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);
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67983): https://edk2.groups.io/g/devel/message/67983
Mute This Topic: https://groups.io/mt/78491894/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-