MdePkg/UefiFileHandleLib: Tolerate more Root handle FileNames

[edk2-devel] [PATCH] MdePkg/UefiFileHandleLib: Tolerate more Root handle FileNames

Posted by Marvin Häuser 4 years, 6 months ago

From: Marvin Haeuser <mhaeuser@outlook.de>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2295

The current implementation of the FileHandleGetFileName() function
assumes that the Root directory always has the FileName '\0'.
However, the only requirement the UEFI specification defines is that
a prepended '\\' must be supported to access files and folders
relative to the Root directory.
This patch removes this assumption and supports constructing valid
paths for any value of FileName for the Root Directory.

In practice, this fixes compatibility issues with File System drivers
that report '\\' as the FileName of the Root directory, which
currently is both generating an invalid path ("\\\\") and resulting
in an EFI_NOT_FOUND result from the CurrentHandle->Open() call.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Marvin Haeuser <mhaeuser@outlook.de>
---
 MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 25 ++++++++++++++------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
index 5dc893833a46..28e28e5f67d5 100644
--- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
+++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
@@ -816,10 +816,25 @@ FileHandleGetFileName (
         Status = EFI_OUT_OF_RESOURCES;

         break;

       } else {

+        //

+        // Prepare to move to the parent directory.

+        // Also determine whether CurrentHandle refers to the Root directory.

+        //

+        Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle, L"..", EFI_FILE_MODE_READ, 0);

         //

         // We got info... do we have a name? if yes precede the current path with it...

         //

-        if (StrLen (FileInfo->FileName) == 0) {

+        if ((StrLen (FileInfo->FileName) == 0) || EFI_ERROR (Status)) {

+          //

+          // Both FileInfo->FileName being '\0' and EFI_ERROR() suggest that

+          // CurrentHandle refers to the Root directory.  As this loop ensures

+          // FullFileName is starting with '\\' at all times, signal success

+          // and exit the loop.

+          // While FileInfo->FileName could theoretically be a value other than

+          // '\0' or '\\', '\\' is guaranteed to be supported by the

+          // specification and hence its value can safely be ignored.

+          //

+          Status = EFI_SUCCESS;

           if (*FullFileName == NULL) {

             ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName != NULL));

             *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0);

@@ -837,15 +852,11 @@ FileHandleGetFileName (
           FreePool(FileInfo);

         }

       }

+

+      FileHandleClose(CurrentHandle);

       //

       // Move to the parent directory

       //

-      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle, L"..", EFI_FILE_MODE_READ, 0);

-      if (EFI_ERROR (Status)) {

-        break;

-      }

-

-      FileHandleClose(CurrentHandle);

       CurrentHandle = NextHigherHandle;

     }

   } else if (Status == EFI_NOT_FOUND) {

-- 
2.23.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49258): https://edk2.groups.io/g/devel/message/49258
Mute This Topic: https://groups.io/mt/35954830/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/UefiFileHandleLib: Tolerate more Root handle FileNames

Posted by Gao, Zhichao 4 years, 6 months ago

This patch makes sense. The patch would not affect the original logic and figure out the failure if the root directory file name is '\\'.
Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Marvin Häuser
> Sent: Sunday, October 20, 2019 8:09 PM
> To: devel@edk2.groups.io
> Cc: vit9696@protonmail.com; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-devel] [PATCH] MdePkg/UefiFileHandleLib: Tolerate more
> Root handle FileNames
> 
> From: Marvin Haeuser <mhaeuser@outlook.de>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2295
> 
> The current implementation of the FileHandleGetFileName() function
> assumes that the Root directory always has the FileName '\0'.
> However, the only requirement the UEFI specification defines is that a
> prepended '\\' must be supported to access files and folders relative to the
> Root directory.
> This patch removes this assumption and supports constructing valid paths for
> any value of FileName for the Root Directory.
> 
> In practice, this fixes compatibility issues with File System drivers that report
> '\\' as the FileName of the Root directory, which currently is both generating
> an invalid path ("\\\\") and resulting in an EFI_NOT_FOUND result from the
> CurrentHandle->Open() call.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Marvin Haeuser <mhaeuser@outlook.de>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 25
> ++++++++++++++------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 5dc893833a46..28e28e5f67d5 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -816,10 +816,25 @@ FileHandleGetFileName (
>          Status = EFI_OUT_OF_RESOURCES;
>          break;
>        } else {
> +        //
> +        // Prepare to move to the parent directory.
> +        // Also determine whether CurrentHandle refers to the Root directory.
> +        //
> +        Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> + L"..", EFI_FILE_MODE_READ, 0);
>          //
>          // We got info... do we have a name? if yes precede the current path
> with it...
>          //
> -        if (StrLen (FileInfo->FileName) == 0) {
> +        if ((StrLen (FileInfo->FileName) == 0) || EFI_ERROR (Status)) {
> +          //
> +          // Both FileInfo->FileName being '\0' and EFI_ERROR() suggest that
> +          // CurrentHandle refers to the Root directory.  As this loop ensures
> +          // FullFileName is starting with '\\' at all times, signal success
> +          // and exit the loop.
> +          // While FileInfo->FileName could theoretically be a value other than
> +          // '\0' or '\\', '\\' is guaranteed to be supported by the
> +          // specification and hence its value can safely be ignored.
> +          //
> +          Status = EFI_SUCCESS;
>            if (*FullFileName == NULL) {
>              ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName !=
> NULL));
>              *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0); @@ -
> 837,15 +852,11 @@ FileHandleGetFileName (
>            FreePool(FileInfo);
>          }
>        }
> +
> +      FileHandleClose(CurrentHandle);
>        //
>        // Move to the parent directory
>        //
> -      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> L"..", EFI_FILE_MODE_READ, 0);
> -      if (EFI_ERROR (Status)) {
> -        break;
> -      }
> -
> -      FileHandleClose(CurrentHandle);
>        CurrentHandle = NextHigherHandle;
>      }
>    } else if (Status == EFI_NOT_FOUND) {
> --
> 2.23.0.windows.1
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49417): https://edk2.groups.io/g/devel/message/49417
Mute This Topic: https://groups.io/mt/35954830/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] MdePkg/UefiFileHandleLib: Tolerate more Root handle FileNames

Posted by Liming Gao 4 years, 5 months ago

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: Gao, Zhichao
>Sent: Thursday, October 24, 2019 10:52 AM
>To: devel@edk2.groups.io; Marvin.Haeuser@outlook.com
>Cc: vit9696@protonmail.com; Kinney, Michael D
><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
>Subject: RE: [PATCH] MdePkg/UefiFileHandleLib: Tolerate more Root handle
>FileNames
>
>This patch makes sense. The patch would not affect the original logic and
>figure out the failure if the root directory file name is '\\'.
>Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Marvin Häuser
>> Sent: Sunday, October 20, 2019 8:09 PM
>> To: devel@edk2.groups.io
>> Cc: vit9696@protonmail.com; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Subject: [edk2-devel] [PATCH] MdePkg/UefiFileHandleLib: Tolerate more
>> Root handle FileNames
>>
>> From: Marvin Haeuser <mhaeuser@outlook.de>
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2295
>>
>> The current implementation of the FileHandleGetFileName() function
>> assumes that the Root directory always has the FileName '\0'.
>> However, the only requirement the UEFI specification defines is that a
>> prepended '\\' must be supported to access files and folders relative to the
>> Root directory.
>> This patch removes this assumption and supports constructing valid paths
>for
>> any value of FileName for the Root Directory.
>>
>> In practice, this fixes compatibility issues with File System drivers that report
>> '\\' as the FileName of the Root directory, which currently is both generating
>> an invalid path ("\\\\") and resulting in an EFI_NOT_FOUND result from the
>> CurrentHandle->Open() call.
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Signed-off-by: Marvin Haeuser <mhaeuser@outlook.de>
>> ---
>>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 25
>> ++++++++++++++------
>>  1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> index 5dc893833a46..28e28e5f67d5 100644
>> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
>> @@ -816,10 +816,25 @@ FileHandleGetFileName (
>>          Status = EFI_OUT_OF_RESOURCES;
>>          break;
>>        } else {
>> +        //
>> +        // Prepare to move to the parent directory.
>> +        // Also determine whether CurrentHandle refers to the Root directory.
>> +        //
>> +        Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
>> + L"..", EFI_FILE_MODE_READ, 0);
>>          //
>>          // We got info... do we have a name? if yes precede the current path
>> with it...
>>          //
>> -        if (StrLen (FileInfo->FileName) == 0) {
>> +        if ((StrLen (FileInfo->FileName) == 0) || EFI_ERROR (Status)) {
>> +          //
>> +          // Both FileInfo->FileName being '\0' and EFI_ERROR() suggest that
>> +          // CurrentHandle refers to the Root directory.  As this loop ensures
>> +          // FullFileName is starting with '\\' at all times, signal success
>> +          // and exit the loop.
>> +          // While FileInfo->FileName could theoretically be a value other than
>> +          // '\0' or '\\', '\\' is guaranteed to be supported by the
>> +          // specification and hence its value can safely be ignored.
>> +          //
>> +          Status = EFI_SUCCESS;
>>            if (*FullFileName == NULL) {
>>              ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName !=
>> NULL));
>>              *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0); @@ -
>> 837,15 +852,11 @@ FileHandleGetFileName (
>>            FreePool(FileInfo);
>>          }
>>        }
>> +
>> +      FileHandleClose(CurrentHandle);
>>        //
>>        // Move to the parent directory
>>        //
>> -      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
>> L"..", EFI_FILE_MODE_READ, 0);
>> -      if (EFI_ERROR (Status)) {
>> -        break;
>> -      }
>> -
>> -      FileHandleClose(CurrentHandle);
>>        CurrentHandle = NextHigherHandle;
>>      }
>>    } else if (Status == EFI_NOT_FOUND) {
>> --
>> 2.23.0.windows.1
>>
>>
>> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#49890): https://edk2.groups.io/g/devel/message/49890
Mute This Topic: https://groups.io/mt/35954830/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-