1. Patchew
  2. EDK2
  3. View series

[edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.

Marvin Häuser posted 1 patch 5 years, 11 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27 ++++++++++++++------
1 file changed, 19 insertions(+), 8 deletions(-)
[edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.
Posted by Marvin Häuser 5 years, 11 months ago 
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 praxis, 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.

V2:
  - Do not change the copyright date as requested.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
---
 MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27 ++++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
index 57aad77bc135..251cbc55edb5 100644
--- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
+++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
@@ -820,10 +820,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);
@@ -841,15 +856,11 @@ FileHandleGetFileName (
           FreePool(FileInfo);
         }
       }
-      //
-      // Move to the parent directory
-      //
-      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle, L"..", EFI_FILE_MODE_READ, 0);
-      if (EFI_ERROR (Status)) {
-        break;
-      }
 
       FileHandleClose(CurrentHandle);
+      //
+      // Move to the parent directory
+      //
       CurrentHandle = NextHigherHandle;
     }
   } else if (Status == EFI_NOT_FOUND) {
-- 
2.17.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.
Posted by Marvin Häuser 5 years, 10 months ago 
Hey Liming and Mike,

Sorry, but I have seen this and another patch to UefiFileHandleLib have not been reviewed and/or merged yet.
Is there anything wrong with the patches?

Thanks,
Marvin.

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Thursday, May 17, 2018 2:42 PM
> To: edk2-devel@lists.01.org
> Cc: michael.d.kinney@intel.com; liming.gao@intel.com
> Subject: [edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory
> determination.
> 
> 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 praxis, 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.
> 
> V2:
>   - Do not change the copyright date as requested.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27
> ++++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 57aad77bc135..251cbc55edb5 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -820,10 +820,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); @@ -
> 841,15 +856,11 @@ FileHandleGetFileName (
>            FreePool(FileInfo);
>          }
>        }
> -      //
> -      // Move to the parent directory
> -      //
> -      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> L"..", EFI_FILE_MODE_READ, 0);
> -      if (EFI_ERROR (Status)) {
> -        break;
> -      }
> 
>        FileHandleClose(CurrentHandle);
> +      //
> +      // Move to the parent directory
> +      //
>        CurrentHandle = NextHigherHandle;
>      }
>    } else if (Status == EFI_NOT_FOUND) {
> --
> 2.17.0.windows.1
> 
> _______________________________________________
> 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
Re: [edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory determination.
Posted by Ni, Ruiyu 5 years, 10 months ago 

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Thursday, May 17, 2018 8:42 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH v2] MdePkg/UefiFileHandleLib: Fix the Root directory
> determination.
> 
> 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 praxis, 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.
> 
> V2:
>   - Do not change the copyright date as requested.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c | 27
> ++++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> index 57aad77bc135..251cbc55edb5 100644
> --- a/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> +++ b/MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.c
> @@ -820,10 +820,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.
> +          //

Per spec, Open("..") request on root directory should return error status.
So I think to keep code simple enough, we could remove the "StrLen(..) == 0"
check.

UEFI Spec 2.7:
".."   Opens the parent directory for the current location. If the location is the
root directory the request will return an error, as there is no parent directory
for the root directory.


> +          Status = EFI_SUCCESS;
>            if (*FullFileName == NULL) {
>              ASSERT((*FullFileName == NULL && Size == 0) || (*FullFileName !=
> NULL));
>              *FullFileName = StrnCatGrowLeft(FullFileName, &Size, L"\\", 0);
> @@ -841,15 +856,11 @@ FileHandleGetFileName (
>            FreePool(FileInfo);
>          }
>        }
> -      //
> -      // Move to the parent directory
> -      //
> -      Status = CurrentHandle->Open (CurrentHandle, &NextHigherHandle,
> L"..", EFI_FILE_MODE_READ, 0);
> -      if (EFI_ERROR (Status)) {
> -        break;
> -      }
> 
>        FileHandleClose(CurrentHandle);
> +      //
> +      // Move to the parent directory
> +      //
>        CurrentHandle = NextHigherHandle;
>      }
>    } else if (Status == EFI_NOT_FOUND) {
> --
> 2.17.0.windows.1
> 
> _______________________________________________
> 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

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.