[PATCH] MdePkg/BaseLib: Fix PathRemoveLastItem to ignore consecutive '\'

[PATCH] MdePkg/BaseLib: Fix PathRemoveLastItem to ignore consecutive '\'

[Jeff Westfahl]

This patch makes PathRemoveLastItem ignore consecutive occurrences of the
'\' path separator.

Consider a path like "FS0:\ABC\DEF\\", noting the consecutive '\' path
separator characters at the end. The expected result of PathRemoveLastItem
on such a path is "FS0:\ABC\". However, what we get is "FS0:\ABC\DEF\".

We can see the result of this behavior with 'ls' in the EFI shell. Go a
couple of folders deep into a filesystem and try 'ls ..\..'. Here's an
example, with a filesystem with folder ABC in the root, with subfolder
DEF.

    FS0:\ABC\DEF\> ls ..
    Directory of: FS0:\ABC\
    05/12/2017  15:46 <DIR>         8,192  .
    05/12/2017  15:46 <DIR>             0  ..
    05/12/2017  15:46 <DIR>         8,192  DEF
              0 File(s)           0 bytes
              3 Dir(s)
    FS0:\ABC\DEF\> ls ..\..
    Directory of: FS0:\ABC\
    05/12/2017  15:46 <DIR>         8,192  .
    05/12/2017  15:46 <DIR>             0  ..
    05/12/2017  15:46 <DIR>         8,192  DEF
              0 File(s)           0 bytes
              3 Dir(s)
    fs0:\ABC\DEF\>

As you can see, 'ls ..\..' lists only the parent folder. This patch
resolves the issue so that 'ls ..\..' lists the grandparent folder.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 MdePkg/Library/BaseLib/FilePaths.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/MdePkg/Library/BaseLib/FilePaths.c b/MdePkg/Library/BaseLib/FilePaths.c
index 203045c..bbaf140 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -37,9 +37,7 @@ PathRemoveLastItem(
       ; Walker != NULL && *Walker != CHAR_NULL
       ; Walker++
      ){
-    if (*Walker == L'\\' && *(Walker + 1) != CHAR_NULL) {
-      LastSlash = Walker+1;
-    } else if (*Walker == L':' && *(Walker + 1) != L'\\' && *(Walker + 1) != CHAR_NULL) {
+    if ((*Walker == L'\\' || *Walker == L':') && *(Walker + 1) != L'\\' && *(Walker + 1) != CHAR_NULL) {
       LastSlash = Walker+1;
     }
   }
-- 
2.7.4

Re: [PATCH] MdePkg/BaseLib: Fix PathRemoveLastItem to ignore consecutive '\'

[Ni, Ruiyu]

Jeff,
PathRemoveLastItem() is expected to be called after PathCleanUpDirectories().
E.g.: what should we expect PathRemoveLastItem() do for "fs0:\a\b\..\"?
So PathRemoveLastItem() expects the incoming path is cleaned.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jeff Westfahl
> Sent: Saturday, May 13, 2017 12:01 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: [edk2] [PATCH] MdePkg/BaseLib: Fix PathRemoveLastItem to ignore
> consecutive '\'
> 
> This patch makes PathRemoveLastItem ignore consecutive occurrences of
> the '\' path separator.
> 
> Consider a path like "FS0:\ABC\DEF\\", noting the consecutive '\' path
> separator characters at the end. The expected result of
> PathRemoveLastItem on such a path is "FS0:\ABC\". However, what we get is
> "FS0:\ABC\DEF\".
> 
> We can see the result of this behavior with 'ls' in the EFI shell. Go a couple of
> folders deep into a filesystem and try 'ls ..\..'. Here's an example, with a
> filesystem with folder ABC in the root, with subfolder DEF.
> 
>     FS0:\ABC\DEF\> ls ..
>     Directory of: FS0:\ABC\
>     05/12/2017  15:46 <DIR>         8,192  .
>     05/12/2017  15:46 <DIR>             0  ..
>     05/12/2017  15:46 <DIR>         8,192  DEF
>               0 File(s)           0 bytes
>               3 Dir(s)
>     FS0:\ABC\DEF\> ls ..\..
>     Directory of: FS0:\ABC\
>     05/12/2017  15:46 <DIR>         8,192  .
>     05/12/2017  15:46 <DIR>             0  ..
>     05/12/2017  15:46 <DIR>         8,192  DEF
>               0 File(s)           0 bytes
>               3 Dir(s)
>     fs0:\ABC\DEF\>
> 
> As you can see, 'ls ..\..' lists only the parent folder. This patch resolves the
> issue so that 'ls ..\..' lists the grandparent folder.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> ---
>  MdePkg/Library/BaseLib/FilePaths.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/FilePaths.c
> b/MdePkg/Library/BaseLib/FilePaths.c
> index 203045c..bbaf140 100644
> --- a/MdePkg/Library/BaseLib/FilePaths.c
> +++ b/MdePkg/Library/BaseLib/FilePaths.c
> @@ -37,9 +37,7 @@ PathRemoveLastItem(
>        ; Walker != NULL && *Walker != CHAR_NULL
>        ; Walker++
>       ){
> -    if (*Walker == L'\\' && *(Walker + 1) != CHAR_NULL) {
> -      LastSlash = Walker+1;
> -    } else if (*Walker == L':' && *(Walker + 1) != L'\\' && *(Walker + 1) !=
> CHAR_NULL) {
> +    if ((*Walker == L'\\' || *Walker == L':') && *(Walker + 1) != L'\\'
> + && *(Walker + 1) != CHAR_NULL) {
>        LastSlash = Walker+1;
>      }
>    }
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdePkg/BaseLib: Fix PathRemoveLastItem to ignore consecutive '\'

[Jeff Westfahl]

Hi Ray,

The problem with 'ls ..\..\' that I'm trying to solve here is the call to 
PathRemoveLastItem from within PathCleanUpDirectories. This intermediate 
path is not clean. Here is a log of the operations performed on a path as 
it goes through PathCleanUpDirectories:

     FS2:\ABC\DEF\> ls ..\..
     ShellCommandRunLs: FullPath FS2:\ABC\DEF\..\..
     ShellCommandRunLs: PathName ..\..
     PathCleanUpDirectories: Entry: FS2:\ABC\DEF\..\..\*
     PathCleanUpDirectories: Stp4a: FS2:\ABC\DEF\
     PathRemoveLastItem: Entry FS2:\ABC\DEF\
     PathRemoveLastItem: Exit1 FS2:\ABC\
     PathCleanUpDirectories: Stp4b: FS2:\ABC\
     PathCleanUpDirectories: Stp4c: FS2:\ABC\\..\*
     PathCleanUpDirectories: Stp4a: FS2:\ABC\\
     PathRemoveLastItem: Entry FS2:\ABC\\
     PathRemoveLastItem: Exit1 FS2:\ABC\
     PathCleanUpDirectories: Stp4b: FS2:\ABC\
     PathCleanUpDirectories: Stp4c: FS2:\ABC\\*
     PathCleanUpDirectories: Step5: FS2:\ABC\*
     PathCleanUpDirectories: Exit:  FS2:\ABC\*

As you can see, on the second iteration through the while loop (Step 4) on 
PathCleanUpDirectories that removes all of the "\..", we pass an unclean 
path to PathRemoveLastItem.

It's difficult to fix this problem in PathCleanUpDirectories because the 
unclean path can be, well, unclean. For example, "FS2:\ABC\DEF\..\\\\..".

Please let me know what you think.

Regards,
Jeff

On Tue, 16 May 2017, Ni, Ruiyu wrote:

> Jeff,
> PathRemoveLastItem() is expected to be called after PathCleanUpDirectories().
> E.g.: what should we expect PathRemoveLastItem() do for "fs0:\a\b\..\"?
> So PathRemoveLastItem() expects the incoming path is cleaned.
>
> Thanks/Ray
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Jeff Westfahl
>> Sent: Saturday, May 13, 2017 12:01 AM
>> To: edk2-devel@lists.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Subject: [edk2] [PATCH] MdePkg/BaseLib: Fix PathRemoveLastItem to ignore
>> consecutive '\'
>>
>> This patch makes PathRemoveLastItem ignore consecutive occurrences of
>> the '\' path separator.
>>
>> Consider a path like "FS0:\ABC\DEF\\", noting the consecutive '\' path
>> separator characters at the end. The expected result of
>> PathRemoveLastItem on such a path is "FS0:\ABC\". However, what we get is
>> "FS0:\ABC\DEF\".
>>
>> We can see the result of this behavior with 'ls' in the EFI shell. Go a couple of
>> folders deep into a filesystem and try 'ls ..\..'. Here's an example, with a
>> filesystem with folder ABC in the root, with subfolder DEF.
>>
>>     FS0:\ABC\DEF\> ls ..
>>     Directory of: FS0:\ABC\
>>     05/12/2017  15:46 <DIR>         8,192  .
>>     05/12/2017  15:46 <DIR>             0  ..
>>     05/12/2017  15:46 <DIR>         8,192  DEF
>>               0 File(s)           0 bytes
>>               3 Dir(s)
>>     FS0:\ABC\DEF\> ls ..\..
>>     Directory of: FS0:\ABC\
>>     05/12/2017  15:46 <DIR>         8,192  .
>>     05/12/2017  15:46 <DIR>             0  ..
>>     05/12/2017  15:46 <DIR>         8,192  DEF
>>               0 File(s)           0 bytes
>>               3 Dir(s)
>>     fs0:\ABC\DEF\>
>>
>> As you can see, 'ls ..\..' lists only the parent folder. This patch resolves the
>> issue so that 'ls ..\..' lists the grandparent folder.
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Jaben Carsey <jaben.carsey@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
>> ---
>>  MdePkg/Library/BaseLib/FilePaths.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/FilePaths.c
>> b/MdePkg/Library/BaseLib/FilePaths.c
>> index 203045c..bbaf140 100644
>> --- a/MdePkg/Library/BaseLib/FilePaths.c
>> +++ b/MdePkg/Library/BaseLib/FilePaths.c
>> @@ -37,9 +37,7 @@ PathRemoveLastItem(
>>        ; Walker != NULL && *Walker != CHAR_NULL
>>        ; Walker++
>>       ){
>> -    if (*Walker == L'\\' && *(Walker + 1) != CHAR_NULL) {
>> -      LastSlash = Walker+1;
>> -    } else if (*Walker == L':' && *(Walker + 1) != L'\\' && *(Walker + 1) !=
>> CHAR_NULL) {
>> +    if ((*Walker == L'\\' || *Walker == L':') && *(Walker + 1) != L'\\'
>> + && *(Walker + 1) != CHAR_NULL) {
>>        LastSlash = Walker+1;
>>      }
>>    }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>