Re: [edk2] A question about shell-application's argument make system blocked;
My recollection is that the code that parses a command line is
basically the same whether the line is in a script or not.
Certainly there are places where that code checks to see if a
script is active, but I don't think it does so when it looks
to eliminate comments.
-Original Message-
From: Carsey, Jaben [mailto:jaben.car...@intel.com]
Sent: Thursday, January 10, 2019 6:03 PM
To: Dailey, Jim
Cc: sssky...@163.com; edk2-devel@lists.01.org
Subject: RE: [edk2] A question about shell-application's argument make system
blocked;
Agreed. That seems like a bug that needs a Bugzilla filed.
My question is if the comment character is valid on a command line typed in
versus in a script file.
> -Original Message-
> From: jim.dai...@dell.com [mailto:jim.dai...@dell.com]
> Sent: Thursday, January 10, 2019 3:34 PM
> To: Carsey, Jaben
> Cc: sssky...@163.com; edk2-devel@lists.01.org
> Subject: RE: [edk2] A question about shell-application's argument make
> system blocked;
> Importance: High
>
> Jaben,
>
> The shell does not parse properly (my opinion) in some instances.
> That is one of the reasons I wrote a separate parser for the shell I
> maintain here at Dell.
>
> One of the areas I feel the parsing is wrong is when an unescaped "#"
> is inside a quoted string:
>
> FS0:\> echo "This should # work."
> Command Error Status: Invalid Parameter
> FS0:\> echo "This should ^# work."
> This should # work.
> FS0:\>
>
> The first command is parsed as if this were the command line:
>
> FS0:\> echo "This should
> Command Error Status: Invalid Parameter
> FS0:\>
>
> I think many people would expect certain characters inside a quoted
> string, like "#" for example, to NOT need escaping. The only ones
> that should need escaping (again IMHO) are: ", ^, and %.
>
> Regards,
> Jim
>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Carsey, Jaben
> Sent: Thursday, January 10, 2019 9:16 AM
> To: krishnaLee; edk2-devel@lists.01.org
> Subject: Re: [edk2] A question about shell-application's argument make
> system blocked;
>
>
> Is this in a script file? I don't remember how "comments" work on raw
> command lines where the user types them.
>
> -Jaben
>
>
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > krishnaLee
> > Sent: Wednesday, January 09, 2019 10:13 PM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] A question about shell-application's argument make system
> > blocked;
> > Importance: High
> >
> > Hi everybody,
> > I meet a question,a special arg can make system blocked,follow is my steps.
> > 1,go to uefi shell v2.2(uefi v2.70),run this application in QEMU-ovmf:
> > testapp.efi
> > 2,the output is "index:0,string:FS0:\testapp.efi"
> >
> >
> > 3,testapp.efi #abc.
> > 4,the output is same as step 2. ///< I had read the uefi shell
> > specification
> > 2.2,the '#' is a comment remark,so I think it is ok.
> >
> >
> > 5 testapp.efi "#abc"
> > 6,the system blocked(dead). ///< I think it is a bug.
> >
> >
> > //follow is the testapp.efi source code:
> > EFI_STATUS
> > EFIAPI
> > UefiMain (
> > IN EFI_HANDLE ImageHandle,
> > IN EFI_SYSTEM_TABLE *SystemTable
> > )
> > {
> > EFI_STATUS status;
> > EFI_SHELL_PARAMETERS_PROTOCOL* param;
> > status=SystemTable->BootServices-
> >
> >HandleProtocol(ImageHandle,&gEfiShellParametersProtocolGuid,¶m);
> > if(status!=EFI_SUCCESS)
> > {
> > return0;
> > }
> >
> >
> > for(UINTN i=0;i< param->Argc;i++)
> > {
> > Print(L"index:%d,string:%s\n",i,param->Argv[i]);
> > }
> >
> >
> > return EFI_SUCCESS;
> > }
> >
> >
> > //test environment:
> > //QEMU v2.10.95 + edk2-2018-ovmf-x64.
> > //shell command line:
> > //"D:\qemu\qemu-system-x86_64.exe" -machine pc-q35-2.9 -pflash
> > "D:\qemu\bios\OVMF_x64_debug.fd" -serial stdio -hda fat:rw:G:\temp -
> net
> > none
> > //end
> >
> >
> >
> >
> >
> >
> > thanks,
> > krishna.
> >
> >
> >
> > ___
> > 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] A question about shell-application's argument make system blocked;
Jaben,
The shell does not parse properly (my opinion) in some instances.
That is one of the reasons I wrote a separate parser for the shell I
maintain here at Dell.
One of the areas I feel the parsing is wrong is when an unescaped "#"
is inside a quoted string:
FS0:\> echo "This should # work."
Command Error Status: Invalid Parameter
FS0:\> echo "This should ^# work."
This should # work.
FS0:\>
The first command is parsed as if this were the command line:
FS0:\> echo "This should
Command Error Status: Invalid Parameter
FS0:\>
I think many people would expect certain characters inside a quoted
string, like "#" for example, to NOT need escaping. The only ones
that should need escaping (again IMHO) are: ", ^, and %.
Regards,
Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Carsey,
Jaben
Sent: Thursday, January 10, 2019 9:16 AM
To: krishnaLee; edk2-devel@lists.01.org
Subject: Re: [edk2] A question about shell-application's argument make system
blocked;
Is this in a script file? I don't remember how "comments" work on raw command
lines where the user types them.
-Jaben
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> krishnaLee
> Sent: Wednesday, January 09, 2019 10:13 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] A question about shell-application's argument make system
> blocked;
> Importance: High
>
> Hi everybody,
> I meet a question,a special arg can make system blocked,follow is my steps.
> 1,go to uefi shell v2.2(uefi v2.70),run this application in QEMU-ovmf:
> testapp.efi
> 2,the output is "index:0,string:FS0:\testapp.efi"
>
>
> 3,testapp.efi #abc.
> 4,the output is same as step 2. ///< I had read the uefi shell specification
> 2.2,the '#' is a comment remark,so I think it is ok.
>
>
> 5 testapp.efi "#abc"
> 6,the system blocked(dead). ///< I think it is a bug.
>
>
> //follow is the testapp.efi source code:
> EFI_STATUS
> EFIAPI
> UefiMain (
> IN EFI_HANDLE ImageHandle,
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> EFI_STATUS status;
> EFI_SHELL_PARAMETERS_PROTOCOL* param;
> status=SystemTable->BootServices-
> >HandleProtocol(ImageHandle,&gEfiShellParametersProtocolGuid,¶m);
> if(status!=EFI_SUCCESS)
> {
> return0;
> }
>
>
> for(UINTN i=0;i< param->Argc;i++)
> {
> Print(L"index:%d,string:%s\n",i,param->Argv[i]);
> }
>
>
> return EFI_SUCCESS;
> }
>
>
> //test environment:
> //QEMU v2.10.95 + edk2-2018-ovmf-x64.
> //shell command line:
> //"D:\qemu\qemu-system-x86_64.exe" -machine pc-q35-2.9 -pflash
> "D:\qemu\bios\OVMF_x64_debug.fd" -serial stdio -hda fat:rw:G:\temp -net
> none
> //end
>
>
>
>
>
>
> thanks,
> krishna.
>
>
>
> ___
> 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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg-BaseLib: PathCleanUpDirectories fix
Liming and Mike,
I don't feel there is any logical issue with this proposed fix.
However, there is some shell code that fails when the fix is in place.
I think that shell code probably should be changed; I'll see if I can
make an acceptable change there first.
Please disregard this patch; I'll likely resubmit some version of it
later on.
Regards,
Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dailey,
Jim
Sent: Friday, November 30, 2018 9:43 AM
To: liming@intel.com; michael.d.kin...@intel.com
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] MdePkg-BaseLib: PathCleanUpDirectories fix
[EXTERNAL EMAIL]
Oops! I think this change may have an issue.
Hold off and I'll let you know if that's the case.
--Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dailey,
Jim
Sent: Friday, November 30, 2018 9:12 AM
To: liming@intel.com; michael.d.kin...@intel.com
Cc: edk2-devel@lists.01.org
Subject: [edk2] [PATCH] MdePkg-BaseLib: PathCleanUpDirectories fix
PathCleanUpDirectories does not handle "\..\" properly; it
returns "\" instead of "". This change fixes that
problem so that "" is returned.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
MdePkg/Library/BaseLib/FilePaths.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseLib/FilePaths.c
b/MdePkg/Library/BaseLib/FilePaths.c
index 92e4c350ff..69e46dd135 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -110,7 +110,12 @@ PathCleanUpDirectories(
((*(TempString + 3) == L'\\') || (*(TempString + 3) == CHAR_NULL))
) {
*(TempString + 1) = CHAR_NULL;
-PathRemoveLastItem(Path);
+if (!PathRemoveLastItem(Path)) {
+ //
+ // We had "\.."
+ //
+ *Path = CHAR_NULL;
+}
if (*(TempString + 3) != CHAR_NULL) {
CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 4));
}
--
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
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg-BaseLib: PathCleanUpDirectories fix
Oops! I think this change may have an issue.
Hold off and I'll let you know if that's the case.
--Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dailey,
Jim
Sent: Friday, November 30, 2018 9:12 AM
To: liming@intel.com; michael.d.kin...@intel.com
Cc: edk2-devel@lists.01.org
Subject: [edk2] [PATCH] MdePkg-BaseLib: PathCleanUpDirectories fix
PathCleanUpDirectories does not handle "\..\" properly; it
returns "\" instead of "". This change fixes that
problem so that "" is returned.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
MdePkg/Library/BaseLib/FilePaths.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseLib/FilePaths.c
b/MdePkg/Library/BaseLib/FilePaths.c
index 92e4c350ff..69e46dd135 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -110,7 +110,12 @@ PathCleanUpDirectories(
((*(TempString + 3) == L'\\') || (*(TempString + 3) == CHAR_NULL))
) {
*(TempString + 1) = CHAR_NULL;
-PathRemoveLastItem(Path);
+if (!PathRemoveLastItem(Path)) {
+ //
+ // We had "\.."
+ //
+ *Path = CHAR_NULL;
+}
if (*(TempString + 3) != CHAR_NULL) {
CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 4));
}
--
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
[edk2] [PATCH] MdePkg-BaseLib: PathCleanUpDirectories fix
PathCleanUpDirectories does not handle "\..\" properly; it
returns "\" instead of "". This change fixes that
problem so that "" is returned.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
MdePkg/Library/BaseLib/FilePaths.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseLib/FilePaths.c
b/MdePkg/Library/BaseLib/FilePaths.c
index 92e4c350ff..69e46dd135 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -110,7 +110,12 @@ PathCleanUpDirectories(
((*(TempString + 3) == L'\\') || (*(TempString + 3) == CHAR_NULL))
) {
*(TempString + 1) = CHAR_NULL;
-PathRemoveLastItem(Path);
+if (!PathRemoveLastItem(Path)) {
+ //
+ // We had "\.."
+ //
+ *Path = CHAR_NULL;
+}
if (*(TempString + 3) != CHAR_NULL) {
CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 4));
}
--
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 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
Yes, Ray, that is the code block that I meant. It serves no purpose since
InputPath cannot possibly be NULL in that part of the code.
I appreciate your help in deleting this block and pushing the rest of the
patch. Also, thanks for your thorough review of this code (and the first
version)!
Regards,
Jim
-Original Message-
From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
Sent: Wednesday, October 31, 2018 4:18 AM
To: Dailey, Jim; edk2-devel@lists.01.org
Cc: Carsey, Jaben
Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
fully-qualify paths
Jim,
I checked the other parts in your patch. They looks good.
But I don't quite understand how to handle the if-statement.
Do you mean to remove the below code block?
//
// Handle the degenerate case where Path was only a file system reference.
// In that case we return the current working directory of the file system.
//
if (InputPath == NULL) {
InputPath = L"";
}
If yes, I can remove it for you and push the patch.
Thanks/Ray
> -Original Message-
> From: jim.dai...@dell.com
> Sent: Tuesday, October 30, 2018 7:32 PM
> To: Ni, Ruiyu ; edk2-devel@lists.01.org
> Cc: Carsey, Jaben
> Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-
> qualify paths
>
> >-Original Message-
> >From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
> >Sent: Tuesday, October 30, 2018 2:33 AM
> >To: Dailey, Jim; edk2-devel@lists.01.org
> >Cc: Carsey, Jaben
> >Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
> >fully-qualify paths
> >
> >
> >> -Original Message-
> >> From: jim.dai...@dell.com
> >> Sent: Tuesday, October 30, 2018 5:15 AM
> >> To: edk2-devel@lists.01.org
> >> Cc: Carsey, Jaben ; Ni, Ruiyu
> >>
> >> Subject: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
> >> fully-qualify paths
> >>
> >> +CHAR16*
> >> +EFIAPI
> >> +FullyQualifyPath(
> >> + IN CONST CHAR16 *Path
> >> + )
> >> +{
> >> + CONST CHAR16 *WorkingPath;
> >> + CONST CHAR16 *InputPath;
> >> + CHAR16 *InputFileSystem;
> >> + UINTNFileSystemCharCount;
> >> + CHAR16 *FullyQualifiedPath;
> >> + UINTNSize;
> >> +
> >> + FullyQualifiedPath = NULL;
> >> +
> >> + ASSERT(Path != NULL);
> >> + //
> >> + // Handle erroneous input when asserts are disabled.
> >> + //
> >> + if (Path == NULL) {
> >> +return NULL;
> >> + }
> >> + //
> >> + // In paths that contain ":", like fs0:dir/file.ext and
> >> + fs2:\fqpath\file.ext, // we have to consider the file system part
> separately from the "path"
> >> part.
> >> + // If there is a file system in the path, we have to get the
> >> + current working // directory for that file system. Then we need to
> >> + use the part of the path // following the ":". If a path does not
> >> contain
> ":", we use it as given.
> >> + //
> >> + InputPath = StrStr(Path, L":");
> >> + if (InputPath != NULL) {
> >> +InputPath++;
> >> +FileSystemCharCount = ((UINTN)InputPath - (UINTN)Path +
> >> sizeof(CHAR16)) / sizeof(CHAR16);
> >> +InputFileSystem = AllocateCopyPool(FileSystemCharCount *
> >> sizeof(CHAR16), Path);
> >> +if (InputFileSystem != NULL) {
> >> + InputFileSystem[FileSystemCharCount - 1] = CHAR_NULL;
> >> +}
> >> +WorkingPath = ShellGetCurrentDir(InputFileSystem);
> >> +SHELL_FREE_NON_NULL(InputFileSystem);
> >> +//
> >> +// Handle the degenerate case where Path was only a file system
> >> reference.
> >> +// In that case we return the current working directory of the file
> system.
> >> +//
> >> +if (InputPath == NULL) {
> >
> >The "InputPath" should not be NULL.
>
> You are correct. It will simply point to an empty string if the input path
> is only
> a file system reference (e.g. "FS0:"). I thoughtlessly confused an empty
> string
> with NULL. :-(
>
> Do you want me to delete that comment and the "if" and resubmit, or,
> assuming the rest of the patch is acceptable, do you want to delete it and
> push the modified patch?
>
> Regards,
> Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
>Sent: Tuesday, October 30, 2018 2:33 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: Carsey, Jaben
>Subject: RE: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
>fully-qualify paths
>
>
>> -Original Message-
>> From: jim.dai...@dell.com
>> Sent: Tuesday, October 30, 2018 5:15 AM
>> To: edk2-devel@lists.01.org
>> Cc: Carsey, Jaben ; Ni, Ruiyu
>> Subject: [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to
>> fully-qualify
>> paths
>>
>> +CHAR16*
>> +EFIAPI
>> +FullyQualifyPath(
>> + IN CONST CHAR16 *Path
>> + )
>> +{
>> + CONST CHAR16 *WorkingPath;
>> + CONST CHAR16 *InputPath;
>> + CHAR16 *InputFileSystem;
>> + UINTNFileSystemCharCount;
>> + CHAR16 *FullyQualifiedPath;
>> + UINTNSize;
>> +
>> + FullyQualifiedPath = NULL;
>> +
>> + ASSERT(Path != NULL);
>> + //
>> + // Handle erroneous input when asserts are disabled.
>> + //
>> + if (Path == NULL) {
>> +return NULL;
>> + }
>> + //
>> + // In paths that contain ":", like fs0:dir/file.ext and
>> fs2:\fqpath\file.ext,
>> + // we have to consider the file system part separately from the "path"
>> part.
>> + // If there is a file system in the path, we have to get the current
>> working
>> + // directory for that file system. Then we need to use the part of the
>> path
>> + // following the ":". If a path does not contain ":", we use it as given.
>> + //
>> + InputPath = StrStr(Path, L":");
>> + if (InputPath != NULL) {
>> +InputPath++;
>> +FileSystemCharCount = ((UINTN)InputPath - (UINTN)Path +
>> sizeof(CHAR16)) / sizeof(CHAR16);
>> +InputFileSystem = AllocateCopyPool(FileSystemCharCount *
>> sizeof(CHAR16), Path);
>> +if (InputFileSystem != NULL) {
>> + InputFileSystem[FileSystemCharCount - 1] = CHAR_NULL;
>> +}
>> +WorkingPath = ShellGetCurrentDir(InputFileSystem);
>> +SHELL_FREE_NON_NULL(InputFileSystem);
>> +//
>> +// Handle the degenerate case where Path was only a file system
>> reference.
>> +// In that case we return the current working directory of the file
>> system.
>> +//
>> +if (InputPath == NULL) {
>
>The "InputPath" should not be NULL.
You are correct. It will simply point to an empty string if the input path
is only a file system reference (e.g. "FS0:"). I thoughtlessly confused an
empty string with NULL. :-(
Do you want me to delete that comment and the "if" and resubmit, or, assuming
the rest of the patch is acceptable, do you want to delete it and push the
modified patch?
Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 1/2] ShellPkg-UefiShellLib: Add a function to fully-qualify paths
Add a function to return a clean, fully-qualified version of some path. This function handles a (possibly "dirty") input path that may or may not include a file system reference. If it does not include a file system reference, then if the input path does not begin with a forward or backward slash, then the input path is relative to the current working directory of the current file system. Otherwise, it is an absolute path within the current file system. If it does include a file system reference, it may be a reference to the current or some other file system. If the file system reference is not immediately followed by a forward or backward slash, then the input path is relative to the current working directory of the given file system. Otherwise, it is an absolute path within the given file system. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jim Dailey --- ShellPkg/Include/Library/ShellLib.h | 35 ++ ShellPkg/Library/UefiShellLib/UefiShellLib.c | 124 ++- 2 files changed, 158 insertions(+), 1 deletion(-) diff --git a/ShellPkg/Include/Library/ShellLib.h b/ShellPkg/Include/Library/ShellLib.h index 92fddc50f5..2ecc5ee006 100644 --- a/ShellPkg/Include/Library/ShellLib.h +++ b/ShellPkg/Include/Library/ShellLib.h @@ -2,6 +2,7 @@ Provides interface to shell functionality for shell commands and applications. Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. + Copyright 2018 Dell Technologies. This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at @@ -35,6 +36,40 @@ extern EFI_SHELL_PARAMETERS_PROTOCOL *gEfiShellParametersProtocol; extern EFI_SHELL_PROTOCOL*gEfiShellProtocol; +/** + Return a clean, fully-qualified version of an input path. If the return value + is non-NULL the caller must free the memory when it is no longer needed. + + If asserts are disabled, and if the input parameter is NULL, NULL is returned. + + If there is not enough memory available to create the fully-qualified path or + a copy of the input path, NULL is returned. + + If there is no working directory, a clean copy of Path is returned. + + Otherwise, the current file system or working directory (as appropriate) is + prepended to Path and the resulting path is cleaned and returned. + + NOTE: If the input path is an empty string, then the current working directory + (if it exists) is returned. In other words, an empty input path is treated + exactly the same as ".". + + @param[in] Path A pointer to some file or directory path. + + @retval NULL The input path is NULL or out of memory. + + @retval non-NULL A pointer to a clean, fully-qualified version of Path. +If there is no working directory, then a pointer to a +clean, but not necessarily fully-qualified version of +Path. The caller must free this memory when it is no +longer needed. +**/ +CHAR16* +EFIAPI +FullyQualifyPath( + IN CONST CHAR16 *Path + ); + /** This function will retrieve the information about the file for the handle specified and store it in allocated pool memory. diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c index f04adbb63f..8467eb8953 100644 --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c @@ -2,7 +2,7 @@ Provides interface to shell functionality for shell commands and applications. (C) Copyright 2016 Hewlett Packard Enterprise Development LP - Copyright 2016 Dell Inc. + Copyright 2016-2018 Dell Technologies. Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License @@ -36,6 +36,128 @@ EFI_HANDLEmEfiShellEnvironment2Handle; FILE_HANDLE_FUNCTION_MAP FileFunctionMap; EFI_UNICODE_COLLATION_PROTOCOL *mUnicodeCollationProtocol; +/** + Return a clean, fully-qualified version of an input path. If the return value + is non-NULL the caller must free the memory when it is no longer needed. + + If asserts are disabled, and if the input parameter is NULL, NULL is returned. + + If there is not enough memory available to create the fully-qualified path or + a copy of the input path, NULL is returned. + + If there is no working directory, a clean copy of Path is returned. + + Otherwise, the current file system or working directory (as appropriate) is + prepended to Path and the resulting path is cleaned and returned. + + NOTE: If the input path is an empty string, then the current working directory + (if it exists) is returned. In other words, an empty input path is
[edk2] [PATCH v2 2/2] ShellPkg-ShellApp: Provide fully-qualified path to shell scripts
Provide fully-qualified path to shell scripts
Section 3.6.2 of version 2.2 of the shell specification requires that
the first positional argument (i.e. arg 0) of a shell script resolves
to "the full path name of the script itself."
Ensure that the startup script and any scripts launched by the shell
meet this requirement.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
ShellPkg/Application/Shell/Shell.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index 6185b6ac80..104f4c8961 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -3,6 +3,7 @@
Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
(C) Copyright 2013-2014 Hewlett-Packard Development Company, L.P.
+ Copyright 2015-2018 Dell Technologies.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -1208,6 +1209,7 @@ DoStartupScript(
UINTN Delay;
EFI_INPUT_KEY Key;
CHAR16*FileStringPath;
+ CHAR16*FullFileStringPath;
UINTN NewSize;
Key.UnicodeChar = CHAR_NULL;
@@ -1275,7 +1277,13 @@ DoStartupScript(
FileStringPath = LocateStartupScript (ImagePath, FilePath);
if (FileStringPath != NULL) {
-Status = RunScriptFile (FileStringPath, NULL, L"",
ShellInfoObject.NewShellParametersProtocol);
+FullFileStringPath = FullyQualifyPath(FileStringPath);
+if (FullFileStringPath == NULL) {
+ Status = RunScriptFile (FileStringPath, NULL, FileStringPath,
ShellInfoObject.NewShellParametersProtocol);
+} else {
+ Status = RunScriptFile (FullFileStringPath, NULL, FullFileStringPath,
ShellInfoObject.NewShellParametersProtocol);
+ FreePool(FullFileStringPath);
+}
FreePool (FileStringPath);
} else {
//
@@ -2429,6 +2437,7 @@ RunCommandOrFile(
EFI_STATUSStatus;
EFI_STATUSStartStatus;
CHAR16*CommandWithPath;
+ CHAR16*FullCommandWithPath;
EFI_DEVICE_PATH_PROTOCOL *DevPath;
SHELL_STATUS CalleeExitStatus;
@@ -2474,7 +2483,13 @@ RunCommandOrFile(
}
switch (Type) {
case Script_File_Name:
- Status = RunScriptFile (CommandWithPath, NULL, CmdLine,
ParamProtocol);
+ FullCommandWithPath = FullyQualifyPath(CommandWithPath);
+ if (FullCommandWithPath == NULL) {
+Status = RunScriptFile (CommandWithPath, NULL, CmdLine,
ParamProtocol);
+ } else {
+Status = RunScriptFile (FullCommandWithPath, NULL, CmdLine,
ParamProtocol);
+FreePool(FullCommandWithPath);
+ }
break;
case Efi_Application:
//
@@ -2812,7 +2827,12 @@ RunScriptFileHandle (
DeleteScriptFileStruct(NewScriptFile);
return (EFI_OUT_OF_RESOURCES);
}
-for (LoopVar = 0 ; LoopVar < 10 && LoopVar < NewScriptFile->Argc;
LoopVar++) {
+//
+// Put the full path of the script file into Argv[0] as required by section
+// 3.6.2 of version 2.2 of the shell specification.
+//
+NewScriptFile->Argv[0] = StrnCatGrow(&NewScriptFile->Argv[0], NULL,
NewScriptFile->ScriptName, 0);
+for (LoopVar = 1 ; LoopVar < 10 && LoopVar < NewScriptFile->Argc;
LoopVar++) {
ASSERT(NewScriptFile->Argv[LoopVar] == NULL);
NewScriptFile->Argv[LoopVar] =
StrnCatGrow(&NewScriptFile->Argv[LoopVar], NULL,
ShellInfoObject.NewShellParametersProtocol->Argv[LoopVar], 0);
if (NewScriptFile->Argv[LoopVar] == NULL) {
--
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 1/2] ShellPkg-Shell App: Provide fully-qualified path to shell scripts
On Thursday, October 25, 2018 12:48 AM ruiyu...@intel.com wrote:
>On 10/25/2018 12:35 AM, jim.dai...@dell.com wrote:
>> Add a function to return the fully-qualified version of some path.
>>
>> ...
>> +CHAR16*
>> +EFIAPI
>> +FullyQualifyPath(
>> + IN OUTCHAR16 **Path
>This API assumes *Path is allocated in heap which may bring unnecessary
>restriction. How about we accept a CONST CHAR16 * Path, quality and
>return a new allocated string?
>The parameter can be "IN CONST CHAR16 *Path".
In the context it is currently used, *Path is allocated in heap. But,
it might be better to handle other types of input too.
>> + if (StrStr(*Path, L":") == NULL) {
>
>Do we need to handle path like "fs0:a.txt"?
>In Windows, it is expanded to + a.txt.
Good catch.
>> +*Path = FullyQualifiedPath;
>
>We can just return the FullQualifiedPath without changing Path.
Agreed.
>Thanks,
>Ray
I'll take a cut at version 2 soon.
Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 2/2] ShellPkg-Shell App: Provide fully-qualified path to shell scripts
Section 3.6.2 of version 2.2 of the shell specification requires that
the first positional argument (i.e. arg 0) of a shell script resolves
to "the full path name of the script itself."
Ensure that the startup script and any scripts launched by the shell
meet this requirement.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
ShellPkg/Application/Shell/Shell.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index 6185b6ac80..fe88177d57 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -3,6 +3,7 @@
Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
(C) Copyright 2013-2014 Hewlett-Packard Development Company, L.P.
+ Copyright 2018 Dell Technologies.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -1275,7 +1276,8 @@ DoStartupScript(
FileStringPath = LocateStartupScript (ImagePath, FilePath);
if (FileStringPath != NULL) {
-Status = RunScriptFile (FileStringPath, NULL, L"",
ShellInfoObject.NewShellParametersProtocol);
+FileStringPath = FullyQualifyPath(&FileStringPath);
+Status = RunScriptFile (FileStringPath, NULL, FileStringPath,
ShellInfoObject.NewShellParametersProtocol);
FreePool (FileStringPath);
} else {
//
@@ -2474,6 +2476,7 @@ RunCommandOrFile(
}
switch (Type) {
case Script_File_Name:
+ CommandWithPath = FullyQualifyPath(&CommandWithPath);
Status = RunScriptFile (CommandWithPath, NULL, CmdLine,
ParamProtocol);
break;
case Efi_Application:
@@ -2812,7 +2815,12 @@ RunScriptFileHandle (
DeleteScriptFileStruct(NewScriptFile);
return (EFI_OUT_OF_RESOURCES);
}
-for (LoopVar = 0 ; LoopVar < 10 && LoopVar < NewScriptFile->Argc;
LoopVar++) {
+//
+// Put the full path of the script file into Argv[0] as required by section
+// 3.6.2 of version 2.2 of the shell specification.
+//
+NewScriptFile->Argv[0] = StrnCatGrow(&NewScriptFile->Argv[0], NULL,
NewScriptFile->ScriptName, 0);
+for (LoopVar = 1 ; LoopVar < 10 && LoopVar < NewScriptFile->Argc;
LoopVar++) {
ASSERT(NewScriptFile->Argv[LoopVar] == NULL);
NewScriptFile->Argv[LoopVar] =
StrnCatGrow(&NewScriptFile->Argv[LoopVar], NULL,
ShellInfoObject.NewShellParametersProtocol->Argv[LoopVar], 0);
if (NewScriptFile->Argv[LoopVar] == NULL) {
--
2.17.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH 1/2] ShellPkg-Shell App: Provide fully-qualified path to shell scripts
Add a function to return the fully-qualified version of some path. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jim Dailey --- ShellPkg/Include/Library/ShellLib.h | 40 + ShellPkg/Library/UefiShellLib/UefiShellLib.c | 93 +++- 2 files changed, 132 insertions(+), 1 deletion(-) diff --git a/ShellPkg/Include/Library/ShellLib.h b/ShellPkg/Include/Library/ShellLib.h index 92fddc50f5..cd7e9c47c3 100644 --- a/ShellPkg/Include/Library/ShellLib.h +++ b/ShellPkg/Include/Library/ShellLib.h @@ -2,6 +2,7 @@ Provides interface to shell functionality for shell commands and applications. Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. + Copyright 2018 Dell Technologies. This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at @@ -35,6 +36,45 @@ extern EFI_SHELL_PARAMETERS_PROTOCOL *gEfiShellParametersProtocol; extern EFI_SHELL_PROTOCOL*gEfiShellProtocol; +/** + Return the fully-qualified version of a relative path or an absolute path that + does not include a file system reference. + + If ASSERTs are disabled, and if the input parameter is NULL or points to NULL, + then NULL is returned. + + If the input path contains a ":", this function assumes that it is part of a + reference to a file system (e.g. "FS0:"). In such a case, Path is cleaned + and returned. + + If there is no working directory or there is not enough memory available to + create the fully-qualified path, Path is cleaned and returned. + + Otherwise, the current file system or working directory (as appropriate) is + prepended to Path. The input Path is freed and the resulting path is cleaned, + assigned to Path, and returned. + + NOTE: If the input path is an empty string, then the current working directory + (if it exists) is returned. In other words, an empty input path is treated + exactly the same as ".". + + @param[in, out] Path On input, a pointer to some file or directory path. On +output, a pointer to the clean and possibly fully- +qualified version of the input path. The input pointer +may be freed and reassigned on output. + + @retval NULL The input pointer or the path itself was NULL. + + @return A pointer to the clean, fully-qualified version of Path. If memory + allocation fails, or if there is no working directory, then a pointer + to the clean, but not necessarily fully-qualified version of Path. +**/ +CHAR16* +EFIAPI +FullyQualifyPath( + IN OUT CHAR16 **Path + ); + /** This function will retrieve the information about the file for the handle specified and store it in allocated pool memory. diff --git a/ShellPkg/Library/UefiShellLib/UefiShellLib.c b/ShellPkg/Library/UefiShellLib/UefiShellLib.c index f04adbb63f..52ca3ce1b1 100644 --- a/ShellPkg/Library/UefiShellLib/UefiShellLib.c +++ b/ShellPkg/Library/UefiShellLib/UefiShellLib.c @@ -2,7 +2,7 @@ Provides interface to shell functionality for shell commands and applications. (C) Copyright 2016 Hewlett Packard Enterprise Development LP - Copyright 2016 Dell Inc. + Copyright 2016-2018 Dell Technologies. Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved. This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License @@ -36,6 +36,97 @@ EFI_HANDLEmEfiShellEnvironment2Handle; FILE_HANDLE_FUNCTION_MAP FileFunctionMap; EFI_UNICODE_COLLATION_PROTOCOL *mUnicodeCollationProtocol; +/** + Return the fully-qualified version of a relative path or an absolute path that + does not include a file system reference. + + If asserts are disabled, and if the input parameter is NULL or points to NULL, + then NULL is returned. + + If the input path contains a ":", this function assumes that it is part of a + reference to a file system (e.g. "FS0:"). In such a case, Path is cleaned + and returned. + + If there is no working directory or there is not enough memory available to + create the fully-qualified path, Path is cleaned and returned. + + Otherwise, the current file system or working directory (as appropriate) is + prepended to Path. The input Path is freed and the resulting path is cleaned, + assigned to Path, and returned. + + NOTE: If the input path is an empty string, then the current working directory + (if it exists) is returned. In other words, an empty input path is treated + exactly the same as ".". + + @param[in, out] Path On input, a pointer to some file or directory path. On +output, a pointer to the clean and possibly fully- +qualified version of the input path. The input pointer +
Re: [edk2] [PATCH] ShellPkg/dmem: Only dump sizeof (EFI_SYSTEM_TABLE) bytes for gST
Is the line:
Size = gST->Hdr.HeaderSize;
possibly a better way of handling this? Either way:
Reviewed-by: Jim Dailey
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Thursday, October 11, 2018 2:53 AM
To: edk2-devel@lists.01.org
Cc: Jaben Carsey
Subject: [edk2] [PATCH] ShellPkg/dmem: Only dump sizeof (EFI_SYSTEM_TABLE)
bytes for gST
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1236
When "dmem" runs without additional arguments, it dumps the memory
content of EFI_SYSTEM_TABLE. But today's implementation dumps 512
bytes. It's not correct because sizeof (EFI_SYSTEM_TABLE) is less
than 512, the 512-read causes page fault exception in a heap-guard
enabled environment.
The patch changes the implementation to only dump
sizeof (EFI_SYSTEM_TABLE) bytes for gST.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Jaben Carsey
---
ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
index f38593a9e9..a4c18c9b68 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Dmem.c
@@ -149,7 +149,7 @@ ShellCommandRunDmem (
Temp1 = ShellCommandLineGetRawValue(Package, 1);
if (Temp1 == NULL) {
Address = gST;
-Size = 512;
+Size= sizeof (*gST);
} else {
if (!ShellIsHexOrDecimalNumber(Temp1, TRUE, FALSE) ||
EFI_ERROR(ShellConvertStringToUint64(Temp1, (UINT64*)&Address, TRUE, FALSE))) {
ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_PARAM_INV),
gShellDebug1HiiHandle, L"dmem", Temp1);
--
2.16.1.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] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
>Sent: Monday, October 8, 2018 9:55 PM
>To: Dailey, Jim
>Cc: michael.d.kin...@intel.com; liming@intel.com; edk2-devel@lists.01.org
>Subject: Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error
>involving "\..\.."
>
>
>On 10/8/2018 9:23 PM, jim.dai...@dell.com wrote:
>>> -Original Message-
diff --git a/MdePkg/Library/BaseLib/FilePaths.c
b/MdePkg/Library/BaseLib/FilePaths.c
index d6f3758ecb..5d3de01894 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -2,6 +2,7 @@
Defines file-path manipulation functions.
Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
+ Copyright (c) 2018, Dell Technologies. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the
BSD License
which accompanies this distribution. The full text of the license
may be found at
@@ -103,7 +104,9 @@ PathCleanUpDirectories(
) {
*(TempString + 1) = CHAR_NULL;
PathRemoveLastItem(Path);
-CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString +
3));
+if (*(TempString + 3)) {
+ CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString
+ 4));
+}
>I just setup a debugging environment to trace the code. Finally I
>understand the issue.
>I agree to change "TempString + 3" to "TempString + 4". This can
>eliminate double slash so in next same loop, the pattern "\\..\\" or
>"\\..\0" can be detected and "last item" can be correctly removed by
>PathRemoveLastItem().
>
>Can you change
> "if (*(TempString + 3))"
> to
> "if (*(TempString + 3) != CHAR_NULL)"?
>
>With the above change, Reviewed-by: Ruiyu Ni
>If you agree, I can modify your patch and push it for you.
>
>--
>Thanks,
>Ray
Sure, I have no problem with `*(TempString + 3) != CHAR_NULL`. Feel
free to make that change and push it.
Thanks,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
Resending, as I never saw this email on the mailing list.
>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
>Sent: Monday, October 8, 2018 2:00 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: michael.d.kin...@intel.com; liming@intel.com
>Subject: Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error
>involving "\..\.."
>
>
>On 10/4/2018 11:03 PM, jim.dai...@dell.com wrote:
>> MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
>>
>> The loop that removes "\..\" errs when multiple "\.." sequences are
>> in the path. Before this change the code would modify a path like
>> "FS0:\efi\tools\..\.." to "FS0:\efi\\.." and then to "FS0:\efi\", but
>> the correct path is "FS0:\".
>>
>> You can test the effect of this change in the shell by setting the
>> current directory to something like FS0:\efi\boot and then executing
>> the command "ls ..\..". Before the change you will see the files in
>> the FS0:\efi directory; after the change, you will see the files in
>> the root directory of FS0:.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jim Dailey
>> ---
>> MdePkg/Library/BaseLib/FilePaths.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/FilePaths.c
>> b/MdePkg/Library/BaseLib/FilePaths.c
>> index d6f3758ecb..5d3de01894 100644
>> --- a/MdePkg/Library/BaseLib/FilePaths.c
>> +++ b/MdePkg/Library/BaseLib/FilePaths.c
>> @@ -2,6 +2,7 @@
>> Defines file-path manipulation functions.
>>
>> Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
>> + Copyright (c) 2018, Dell Technologies. All rights reserved.
>> This program and the accompanying materials
>> are licensed and made available under the terms and conditions of the
>> BSD License
>> which accompanies this distribution. The full text of the license may
>> be found at
>> @@ -103,7 +104,9 @@ PathCleanUpDirectories(
>> ) {
>> *(TempString + 1) = CHAR_NULL;
>> PathRemoveLastItem(Path);
>> -CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString +
>> 3));
>> +if (*(TempString + 3)) {
>> + CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString +
>> 4));
>> +}
>> }
>>
>> //
>>
>Jim,
>Are you fixing a corner case bug introduced by following commit:
>
> > SHA-1: bb99e3282c9e69fbd6365d117c58d15589e34c5d
>* MdePkg/BaseLib: Fix PathCleanUpDirectories to correctly handle "\.\"
>
> > The old code incorrectly cleans path like "fs0:\abc\.\.." to
> > "fs0:\abc", instead of "fs0:\"
>
> > The patch fixes this bug.
Honestly, because of the scope of bb99e328, I cannot say for sure, but
my inclination is to answer "No."
The current fix is strictly related to handling paths that include two
or more references to the previous directory, whether or not one of them
abuts a reference to the current directory (as in the bb99e328 fix).
Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable
>-Original Message- >From: Ni, Ruiyu [mailto:ruiyu...@intel.com] >Sent: Monday, October 8, 2018 1:42 AM >To: Dailey, Jim; edk2-devel@lists.01.org >Cc: jaben.car...@intel.com >Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment >variable > > >On 10/4/2018 12:02 AM, jim.dai...@dell.com wrote: >> Create a homefilesystem environment variable whose value is the file >> system on which the executing shell is located. For example: "FS14:". >> >Jim, >Creating spec-undefined "homefilesystem" ENV variable is not a good idea >in my opinion. >Can you submit a Shell Spec change and change the implementation once >the spec change is approved? Ray, I think you and Jaben should get together on this: >>-Original Message- >>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo >>Ersek >>Sent: Thursday, October 4, 2018 12:07 PM >>To: Carsey, Jaben; Dailey, Jim; edk2-devel@lists.01.org >>Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment >>variable >> >>Is it OK with the UEFI shell spec to define a shell variable called >>"homefilesystem"? I seem to remember that edk2-specific options for >>standard UEFI shell commands generally start with an underscore, to >>avoid clashing with the standard namespace. Does that apply to shell >>variables perhaps? (This is mostly for my own education.) >> >>> -Original Message- >>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >>> Carsey, Jaben >>> Sent: Thursday, October 4, 2018 3:54 PM >>> To: Andrew Fish; Laszlo Ersek >>> Cc: edk2-devel@lists.01.org >>> Subject: Re: [edk2] [PATCH] ShellPkg: Create a homefilesystem environment >>> variable >>> >>> Laszlo, >>> >>> The leading "_" was required for out of spec, but built in, commands. >>> The spec has no restrictions on environment variables except some have >>> special meaning and may be read only. >>> Besides, if I were you and didn't have a lot of free time, I wouldn't unleash me on shell spec changes! :-) >-- >Thanks, >Ray Regards, Jim ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
>-Original Message-
>From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
>Sent: Monday, October 8, 2018 2:00 AM
>To: Dailey, Jim; edk2-devel@lists.01.org
>Cc: michael.d.kin...@intel.com; liming@intel.com
>Subject: Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error
>involving "\..\.."
>
>
>On 10/4/2018 11:03 PM, jim.dai...@dell.com wrote:
>> MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
>>
>> The loop that removes "\..\" errs when multiple "\.." sequences are
>> in the path. Before this change the code would modify a path like
>> "FS0:\efi\tools\..\.." to "FS0:\efi\\.." and then to "FS0:\efi\", but
>> the correct path is "FS0:\".
>>
>> You can test the effect of this change in the shell by setting the
>> current directory to something like FS0:\efi\boot and then executing
>> the command "ls ..\..". Before the change you will see the files in
>> the FS0:\efi directory; after the change, you will see the files in
>> the root directory of FS0:.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jim Dailey
>> ---
>> MdePkg/Library/BaseLib/FilePaths.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/FilePaths.c
>> b/MdePkg/Library/BaseLib/FilePaths.c
>> index d6f3758ecb..5d3de01894 100644
>> --- a/MdePkg/Library/BaseLib/FilePaths.c
>> +++ b/MdePkg/Library/BaseLib/FilePaths.c
>> @@ -2,6 +2,7 @@
>> Defines file-path manipulation functions.
>>
>> Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
>> + Copyright (c) 2018, Dell Technologies. All rights reserved.
>> This program and the accompanying materials
>> are licensed and made available under the terms and conditions of the
>> BSD License
>> which accompanies this distribution. The full text of the license may
>> be found at
>> @@ -103,7 +104,9 @@ PathCleanUpDirectories(
>> ) {
>> *(TempString + 1) = CHAR_NULL;
>> PathRemoveLastItem(Path);
>> -CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString +
>> 3));
>> +if (*(TempString + 3)) {
>> + CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString +
>> 4));
>> +}
>> }
>>
>> //
>>
>Jim,
>Are you fixing a corner case bug introduced by following commit:
>
> > SHA-1: bb99e3282c9e69fbd6365d117c58d15589e34c5d
>* MdePkg/BaseLib: Fix PathCleanUpDirectories to correctly handle "\.\"
>
> > The old code incorrectly cleans path like "fs0:\abc\.\.." to
> > "fs0:\abc", instead of "fs0:\"
>
> > The patch fixes this bug.
Honestly, because of the scope of bb99e328, I cannot say for sure, but
my inclination is to answer "No."
The current fix is strictly related to handling paths that include two
or more references to the previous directory, whether or not one of them
abuts a reference to the current directory (as in the bb99e328 fix).
Regards,
Jim
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() issue with "\\..\\.."
Replace multiple, consecutive "\" characters prior to other processing
involving "\" characters. This fixes an issue where "\\..\\..",
"//..//..", and similar input paths are not cleaned properly.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
MdePkg/Library/BaseLib/FilePaths.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/MdePkg/Library/BaseLib/FilePaths.c
b/MdePkg/Library/BaseLib/FilePaths.c
index d6f3758ecb..c5ca0a3b77 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -2,6 +2,7 @@
Defines file-path manipulation functions.
Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
+ Copyright (c) 2018, Dell Technologies. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -85,6 +86,13 @@ PathCleanUpDirectories(
}
}
+ //
+ // Replace the "\\" with "\"
+ //
+ while ((TempString = StrStr (Path, L"")) != NULL) {
+CopyMem (TempString, TempString + 1, StrSize (TempString + 1));
+ }
+
//
// Remove all the "\.". E.g.: fs0:\abc\.\def\.
//
@@ -106,13 +114,6 @@ PathCleanUpDirectories(
CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 3));
}
- //
- // Replace the "\\" with "\"
- //
- while ((TempString = StrStr (Path, L"")) != NULL) {
-CopyMem (TempString, TempString + 1, StrSize (TempString + 1));
- }
-
return Path;
}
--
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] ShellPkg: Create a homefilesystem environment variable
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo >Ersek I'll attempt answer some of your questions, but Jaben may have to answer some of them (like his commit speed :-) or questions about what the shell spec allows). > >So my first question would have been, what if the shell is memory mapped >(from a firmware volume), but the platform doesn't expose firmware >filesystems (FFSs) as EFI simple file system protocol instances? In that >case, the "file system on which the executing shell is located" seems >ill-defined. In such a case homefilesystem will not get defined, I think. execute %homefilesystem% to set the cwd to the root of the file system where the shell is located. > >I think the commit message here misses a "CD" command. The shell does not handle "cd fsN:". But "fsN:" does work. I suppose one could always add a "cd \" after "%hoemfilesystem%", but I think it will not have any effect in most (all?) cases where homefilesystem is defined. +InternalEfiShellSetEnv(L"homefilesystem", StartupScriptPath, TRUE); + > >Again, this refers to the startup script, not the shell itself. The variable's name implies the startup script, but at the point it is used, it contains only the file system where the shell itself was found. Code following this continues to modify the variable's value until it eventually does point to where the startup script *might* be. >Thanks >Laszlo > Regards, Jim ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] ShellPkg-Cd: Ensure all valid cd targets are handled properly
ShellPkg-Cd: Ensure all valid cd targets are handled properly
Make sure that PathCleanUpDirectories() is called on all valid targets
of the cd command.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
index 79dd2096f4..1eb7056aee 100644
--- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
+++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cd.c
@@ -4,6 +4,7 @@
(C) Copyright 2016 Hewlett Packard Enterprise Development LP
(C) Copyright 2015 Hewlett-Packard Development Company, L.P.
Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
+ Copyright (c) 2018, Dell Technologies. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -261,9 +262,6 @@ ShellCommandRunCd (
if (Param1Copy != NULL && IsCurrentFileSystem (Param1Copy, Cwd)) {
Status = ReplaceDriveWithCwd (&Param1Copy,Cwd);
- if (!EFI_ERROR (Status)) {
-Param1Copy = PathCleanUpDirectories (Param1Copy);
- }
} else {
//
// Can't use cd command to change filesystem.
@@ -302,13 +300,15 @@ ShellCommandRunCd (
StrCatS (TempBuffer, TotalSize / sizeof (CHAR16), Param1Copy);
FreePool (Param1Copy);
-Param1Copy = PathCleanUpDirectories (TempBuffer);
+Param1Copy = TempBuffer;
+TempBuffer = NULL;
}
}
}
}
if (!EFI_ERROR(Status)) {
+ Param1Copy = PathCleanUpDirectories (Param1Copy);
Status = ExtractDriveAndPath (Param1Copy, &Drive, &Path);
}
--
2.17.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
MdePkg-BaseLib: Fix PathCleanUpDirectories() error involving "\..\.."
The loop that removes "\..\" errs when multiple "\.." sequences are
in the path. Before this change the code would modify a path like
"FS0:\efi\tools\..\.." to "FS0:\efi\\.." and then to "FS0:\efi\", but
the correct path is "FS0:\".
You can test the effect of this change in the shell by setting the
current directory to something like FS0:\efi\boot and then executing
the command "ls ..\..". Before the change you will see the files in
the FS0:\efi directory; after the change, you will see the files in
the root directory of FS0:.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jim Dailey
---
MdePkg/Library/BaseLib/FilePaths.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Library/BaseLib/FilePaths.c
b/MdePkg/Library/BaseLib/FilePaths.c
index d6f3758ecb..5d3de01894 100644
--- a/MdePkg/Library/BaseLib/FilePaths.c
+++ b/MdePkg/Library/BaseLib/FilePaths.c
@@ -2,6 +2,7 @@
Defines file-path manipulation functions.
Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
+ Copyright (c) 2018, Dell Technologies. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -103,7 +104,9 @@ PathCleanUpDirectories(
) {
*(TempString + 1) = CHAR_NULL;
PathRemoveLastItem(Path);
-CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 3));
+if (*(TempString + 3)) {
+ CopyMem (Path + StrLen (Path), TempString + 4, StrSize (TempString + 4));
+}
}
//
--
2.17.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] ShellPkg: Create a homefilesystem environment variable
Create a homefilesystem environment variable whose value is the file system on which the executing shell is located. For example: "FS14:". This eliminates the need for people to have to try and find the "boot" file system in their startup script. After this change they can simply execute %homefilesystem% to set the cwd to the root of the file system where the shell is located. A future enhancement could be to add "homefilesystem" to the list of predefined, read-only variables listed in the EfiShellSetEnv function of file ShellProtocol.c Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jim Dailey --- ShellPkg/Application/Shell/Shell.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ShellPkg/Application/Shell/Shell.c b/ShellPkg/Application/Shell/Shell.c index 3f3bcbb4b0..6185b6ac80 100644 --- a/ShellPkg/Application/Shell/Shell.c +++ b/ShellPkg/Application/Shell/Shell.c @@ -1169,6 +1169,8 @@ LocateStartupScript ( *TempSpot = CHAR_NULL; } +InternalEfiShellSetEnv(L"homefilesystem", StartupScriptPath, TRUE); + StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size, ((FILEPATH_DEVICE_PATH *)FileDevicePath)->PathName, 0); PathRemoveLastItem (StartupScriptPath); StartupScriptPath = StrnCatGrow (&StartupScriptPath, &Size, mStartupScript, 0); -- 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] ShellPkg Shell: Remove redundant functions
The InvalidChars[] array is only used in function IsValidCommandName().
The array should be deleted also, I think.
Regards,
Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of shenglei
Sent: Thursday, August 9, 2018 12:42 AM
To: edk2-devel@lists.01.org
Cc: Jaben Carsey; Ruiyu Ni
Subject: [edk2] [PATCH] ShellPkg Shell: Remove redundant functions
The redundant functions which are never called have been
removed. They are InternalShellProtocolDebugPrintMessage,
UpdateFileName,RemoveFileTag and IsValidCommandName.
https://bugzilla.tianocore.org/show_bug.cgi?id=1066
Cc: Jaben Carsey
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: shenglei
---
ShellPkg/Application/Shell/Shell.c| 29 ---
ShellPkg/Application/Shell/Shell.h| 13 ---
.../Shell/ShellParametersProtocol.c | 24 --
ShellPkg/Application/Shell/ShellProtocol.c| 81 +--
4 files changed, 1 insertion(+), 146 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index 47ae3c373c..397cfd1994 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -2752,35 +2752,6 @@ RunCommand(
STATIC CONST UINT16 InvalidChars[] = {L'*', L'?', L'<', L'>', L'\\', L'/',
L'\"', 0x0001, 0x0002};
-/**
- Function determines if the CommandName COULD be a valid command. It does
not determine whether
- this is a valid command. It only checks for invalid characters.
-
- @param[in] CommandNameThe name to check
-
- @retval TRUE CommandName could be a command name
- @retval FALSE CommandName could not be a valid command name
-**/
-BOOLEAN
-IsValidCommandName(
- IN CONST CHAR16 *CommandName
- )
-{
- UINTN Count;
- if (CommandName == NULL) {
-ASSERT(FALSE);
-return (FALSE);
- }
- for ( Count = 0
- ; Count < sizeof(InvalidChars) / sizeof(InvalidChars[0])
- ; Count++
- ){
-if (ScanMem16(CommandName, StrSize(CommandName), InvalidChars[Count]) !=
NULL) {
- return (FALSE);
-}
- }
- return (TRUE);
-}
/**
Function to process a NSH script file via SHELL_FILE_HANDLE.
diff --git a/ShellPkg/Application/Shell/Shell.h
b/ShellPkg/Application/Shell/Shell.h
index 69b19c6a2d..bad8f08d47 100644
--- a/ShellPkg/Application/Shell/Shell.h
+++ b/ShellPkg/Application/Shell/Shell.h
@@ -309,19 +309,6 @@ RunShellCommand(
OUT EFI_STATUS*CommandStatus
);
-/**
- Function determines if the CommandName COULD be a valid command. It does
not determine whether
- this is a valid command. It only checks for invalid characters.
-
- @param[in] CommandNameThe name to check
-
- @retval TRUE CommandName could be a command name
- @retval FALSE CommandName could not be a valid command name
-**/
-BOOLEAN
-IsValidCommandName(
- IN CONST CHAR16 *CommandName
- );
/**
Function to process a NSH script file via SHELL_FILE_HANDLE.
diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c
b/ShellPkg/Application/Shell/ShellParametersProtocol.c
index 90889a3725..a21c690518 100644
--- a/ShellPkg/Application/Shell/ShellParametersProtocol.c
+++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c
@@ -626,30 +626,6 @@ FixVarName (
return (FixFileName(Copy));
}
-/**
- Remove the unicode file tag from the begining of the file buffer since that
will not be
- used by StdIn.
-
- @param[in] HandlePointer to the handle of the file to be processed.
-
- @retval EFI_SUCCESS The unicode file tag has been moved successfully.
-**/
-EFI_STATUS
-RemoveFileTag(
- IN SHELL_FILE_HANDLE *Handle
- )
-{
- UINTN CharSize;
- CHAR16CharBuffer;
-
- CharSize= sizeof(CHAR16);
- CharBuffer = 0;
- gEfiShellProtocol->ReadFile(*Handle, &CharSize, &CharBuffer);
- if (CharBuffer != gUnicodeFileTag) {
-gEfiShellProtocol->SetFilePosition(*Handle, 0);
- }
- return (EFI_SUCCESS);
-}
/**
Write the unicode file tag to the specified file.
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
b/ShellPkg/Application/Shell/ShellProtocol.c
index f2ca2029e3..8cf924b384 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -98,40 +98,6 @@ InternalShellProtocolIsSimpleFileSystemPresent(
return (FALSE);
}
-/**
- Internal worker debug helper function to print out maps as they are added.
-
- @param[in] Mappingstring mapping that has been added
- @param[in] DevicePath pointer to device path that has been mapped.
-
- @retval EFI_SUCCESS the operation was successful.
- @return other an error ocurred
-
- @sa LocateHandle
- @sa OpenProtocol
-**/
-EFI_STATUS
-InternalShellProtocolDebugPrintMessage (
- IN CONST CHAR16 *Mapping,
- IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath
- )
-{
- EFI_STATUSStatus;
- C
Re: [edk2] [PATCH v2] ShellPkg/set: Fix EfiShellSetEnv to use case sensitive compare
Reviewed-by: Jim Dailey
-Original Message-
From: Ruiyu Ni [mailto:ruiyu...@intel.com]
Sent: Wednesday, August 8, 2018 12:04 AM
To: edk2-devel@lists.01.org
Cc: Jaben Carsey; Dailey, Jim
Subject: [PATCH v2] ShellPkg/set: Fix EfiShellSetEnv to use case sensitive
compare
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=777
Per Shell spec, the environment variable has a case-sensitive name.
But today's implementation of EfiShellSetEnv() compares the
environment variable name case insensitively, which causes variable
like "CWD" cannot be set due to "cwd" is pre-defined variable.
The patch fixes this issue.
The EfiShellGetEnv() doesn't have such issue because it will
call into ShellFindEnvVarInList() which uses StrCmp().
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Jaben Carsey
Cc: Jim Dailey
---
ShellPkg/Application/Shell/ShellProtocol.c | 39 +++---
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
b/ShellPkg/Application/Shell/ShellProtocol.c
index f2ca2029e3..9e9e6dc052 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -2924,36 +2924,15 @@ EfiShellSetEnv(
//
// Make sure we dont 'set' a predefined read only variable
//
- if (gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-L"cwd") == 0
-||gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-L"Lasterror") == 0
-||gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-L"profiles") == 0
-||gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-L"uefishellsupport") == 0
-||gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-L"uefishellversion") == 0
-||gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-L"uefiversion") == 0
-||(!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoNest &&
- gUnicodeCollation->StriColl(
-gUnicodeCollation,
-(CHAR16*)Name,
-(CHAR16*)mNoNestingEnvVarName) == 0)
- ){
+ if ((StrCmp (Name, L"cwd") == 0) ||
+ (StrCmp (Name, L"lasterror") == 0) ||
+ (StrCmp (Name, L"profiles") == 0) ||
+ (StrCmp (Name, L"uefishellsupport") == 0) ||
+ (StrCmp (Name, L"uefishellversion") == 0) ||
+ (StrCmp (Name, L"uefiversion") == 0) ||
+ (!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoNest &&
+ StrCmp (Name, mNoNestingEnvVarName) == 0)
+ ) {
return (EFI_INVALID_PARAMETER);
}
return (InternalEfiShellSetEnv(Name, Value, Volatile));
--
2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ShellPkg/set: Fix EfiShellSetEnv to use case sensitive compare
Ray,
Table 8 in version 2.2 of the Shell Spec says "lasterror" (lower-case L). Why
compare to "Lasterror"?
Regards,
Jim
-Original Message-
From: Carsey, Jaben [mailto:jaben.car...@intel.com]
Sent: Tuesday, August 7, 2018 12:57 PM
To: Ni, Ruiyu; edk2-devel@lists.01.org
Cc: Dailey, Jim
Subject: RE: [PATCH] ShellPkg/set: Fix EfiShellSetEnv to use case sensitive
compare
Reviewed-by: Jaben Carsey
> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, August 07, 2018 2:14 AM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben ; Jim Dailey
>
> Subject: [PATCH] ShellPkg/set: Fix EfiShellSetEnv to use case sensitive
> compare
> Importance: High
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=777
>
> Per Shell spec, the environment variable has a case-sensitive name.
> But today's implementation of EfiShellSetEnv() compares the
> environment variable name case insensitively, which causes variable
> like "CWD" cannot be set due to "cwd" is pre-defined variable.
>
> The patch fixes this issue.
>
> The EfiShellGetEnv() doesn't have such issue because it will
> call into ShellFindEnvVarInList() which uses StrCmp().
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni
> Cc: Jaben Carsey
> Cc: Jim Dailey
> ---
> ShellPkg/Application/Shell/ShellProtocol.c | 39
> +++---
> 1 file changed, 9 insertions(+), 30 deletions(-)
>
> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> b/ShellPkg/Application/Shell/ShellProtocol.c
> index f2ca2029e3..767cbc99a0 100644
> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> @@ -2924,36 +2924,15 @@ EfiShellSetEnv(
>//
>// Make sure we dont 'set' a predefined read only variable
>//
> - if (gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -L"cwd") == 0
> -||gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -L"Lasterror") == 0
> -||gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -L"profiles") == 0
> -||gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -L"uefishellsupport") == 0
> -||gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -L"uefishellversion") == 0
> -||gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -L"uefiversion") == 0
> -||(!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoNest &&
> - gUnicodeCollation->StriColl(
> -gUnicodeCollation,
> -(CHAR16*)Name,
> -(CHAR16*)mNoNestingEnvVarName) == 0)
> - ){
> + if ((StrCmp (Name, L"cwd") == 0) ||
> + (StrCmp (Name, L"Lasterror") == 0) ||
> + (StrCmp (Name, L"profiles") == 0) ||
> + (StrCmp (Name, L"uefishellsupport") == 0) ||
> + (StrCmp (Name, L"uefishellversion") == 0) ||
> + (StrCmp (Name, L"uefiversion") == 0) ||
> + (!ShellInfoObject.ShellInitSettings.BitUnion.Bits.NoNest &&
> + StrCmp (Name, mNoNestingEnvVarName) == 0)
> + ) {
> return (EFI_INVALID_PARAMETER);
>}
>return (InternalEfiShellSetEnv(Name, Value, Volatile));
> --
> 2.16.1.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] How to Interpret ReadKeyStrokeEX Data
Thanks, Mike. I figured that out yesterday. I think the only problem now is that any shell built with the original change to edit will not work as expected if it is run on a system whose BIOS was created before the second change was made (i.e. pretty much every existing UEFI BIOS in the world!). Since the shell is a stand-alone tool that is not necessarily tied to any particular machine, I think that it is likely that more than a few people may run into this problem in the future. Regards, Jim -Original Message- From: Rothman, Michael A [mailto:michael.a.roth...@intel.com] Sent: Tuesday, June 5, 2018 10:38 PM To: Rothman, Michael A; Dailey, Jim; Ni, Ruiyu Cc: Carsey, Jaben; edk2-devel@lists.01.org; fel...@mail.ru Subject: RE: How to Interpret ReadKeyStrokeEX Data Jim, I think the problem you're seeing is that the USB keyboard driver you're using is downrev and needs to be updated. If you look at https://github.com/tianocore/edk2/commit/dd190645eb43424706eb1709d0032c69a1935d9f there was a fix checked in to address exactly the issue you're running into. It's basically a symptom of running a new shell without a correspondingly updated keyboard driver. The new shell in effect exposed a latent bug. Hope that helps. Thanks, Mike Rothman (迈克 罗斯曼 / माइकल रोथ्मेन् / Михаил Ротман / משה רוטמן) רועה עיקרי של חתולים -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Rothman, Michael A Sent: Monday, June 4, 2018 10:31 AM To: jim.dai...@dell.com; Ni, Ruiyu Cc: Carsey, Jaben ; edk2-devel@lists.01.org; fel...@mail.ru Subject: Re: [edk2] How to Interpret ReadKeyStrokeEX Data Since I'm largely the person who might be to blame for the language and intent here and I’ll focus on the spec-related item. See my comments below. Thanks, Mike Rothman (迈克 罗斯曼 / माइकल रोथ्मेन् / Михаил Ротман / משה רוטמן) רועה עיקרי של חתולים -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of jim.dai...@dell.com Sent: Friday, June 1, 2018 11:27 AM To: Ni, Ruiyu Cc: Carsey, Jaben ; edk2-devel@lists.01.org; fel...@mail.ru Subject: [edk2] How to Interpret ReadKeyStrokeEX Data (Subject changed) I guess this is a question of UEFI spec interpretation. In the Console I/O Protocol description of Version 2.5 of the spec (page 456), it says: If the Scan Code is set to 0x00 then the Unicode character is valid and should be used. To me that clearly says it all. The shift modifier is a don't care when the scan code is zero. And, this change in the shell code seems to be a violation of that statement. However, I also see some confusing (and grammatically incorrect) text in the description of the ReadKeyStrokeEx() function of the simple text in protocol that I am guessing is related to this change (*emphasis* mine): When interpreting the data from this function, it should be noted that if a class of printable characters that are normally *adjusted* by shift modifiers (e.g. Shift Key + "f" key) would be presented solely as a KeyData.Key.UnicodeChar without the associated shift state. What I think the spec is trying to say here is that for A-Z and a-z, the consumer does NOT need to look at the shift state to tell "A" from "a", for example, because the Unicode character will be either "A" or "a" as appropriate. n No, it is any key that would create a displayable character that adjusts based on the shift state. Realize that the users of ReadKeyStroke/ReadKeyStrokeEx fall back to a common denominator of Scan Code or Unicode Character. So if someone types a shift 4, the underlying keyboard layout that the keyboard driver controls would dictate how that gets translated. On my keyboard in the US it turns into a “$” symbol, while someone in Europe may very well have a software-defined keyboard layout which translates the same keystroke to a “€” symbol. That of course applies to the many characters you specified (A-F, a-f) and many others. n The text above is intended to imply what it says, “a class of printable characters … would be presented solely as a KeyData.Key.UnicodeChar without the associated shift state. This makes consumers of both the Ex and Non-Ex variant of ReadKeyStroke able to use the same logic to test for any shiftable characters by simply looking at the Unicode value. I’d note the shift and toggle states (which are only available on Ex) are there not so much for interpreting the translated key but to maximize flexibility associated with keyboard mapping as a UEFI application. I think saying this is unnecessary simply because the earlier statement (If the Scan Code is set to 0x00 then the Unicode character is valid and should be used.) covers this case. Further, I believe this text applies only to the A-Z keys because their corresponding characters are *adjusted* (to upper case) when the shift key is pressed. That is, if y
Re: [edk2] How to Interpret ReadKeyStrokeEX Data
Yes! Sorry if I was unclear.
-Original Message-
From: Carsey, Jaben [mailto:jaben.car...@intel.com]
Sent: Monday, June 4, 2018 11:22 AM
To: Dailey, Jim
Cc: af...@apple.com; Ni, Ruiyu; edk2-devel@lists.01.org; fel...@mail.ru;
Rothman, Michael A
Subject: Re: [edk2] How to Interpret ReadKeyStrokeEX Data
Jim,
I think I see what you mean. If scan code is 0, then don't test the shift
state and just use the character raw?
Jaben
Sent from my iPad
> On Jun 4, 2018, at 9:02 AM, "jim.dai...@dell.com" wrote:
>
> Please disregard the stupid "Confidential" line that our email tool
> adds but hides from me when I send text. :-(
>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Dailey, Jim
> Sent: Monday, June 4, 2018 11:00 AM
> To: af...@apple.com
> Cc: ruiyu...@intel.com; jaben.car...@intel.com; edk2-devel@lists.01.org;
> fel...@mail.ru
> Subject: Re: [edk2] How to Interpret ReadKeyStrokeEX Data
>
> Dell - Internal Use - Confidential <=== THIS IS BOGUS !!
>
>> From: af...@apple.com [mailto:af...@apple.com]
>>
>> The big picture difference is the original SimpleTextIn was the least common
>> denominator with a serial terminal. The Ex version added more info about
>> keyboards, so richer info on modifier keys.
>
> I get that. But I fail to see how that affects SimpleTextInEx behavior or
> what the UEFI spec has to say about it.
>
> As I said earlier, the question I am raising is when SimpleTextInEx returns
> something like:
>
> Scan Code = 0
> Unicode Char = 0x0023 ("#")
> Shift Information = 0x8001 (right shift pressed)
>
> is it correct for the editor to reject this as an invalid key?
>
> I say, no, it would be wrong to reject this data because the scan code
> is 0 and, therefore, the Unicode character is valid and should be used.
> ___
> 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] How to Interpret ReadKeyStrokeEX Data
Please disregard the stupid "Confidential" line that our email tool
adds but hides from me when I send text. :-(
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dailey,
Jim
Sent: Monday, June 4, 2018 11:00 AM
To: af...@apple.com
Cc: ruiyu...@intel.com; jaben.car...@intel.com; edk2-devel@lists.01.org;
fel...@mail.ru
Subject: Re: [edk2] How to Interpret ReadKeyStrokeEX Data
Dell - Internal Use - Confidential <=== THIS IS BOGUS !!
> From: af...@apple.com [mailto:af...@apple.com]
>
> The big picture difference is the original SimpleTextIn was the least common
> denominator with a serial terminal. The Ex version added more info about
> keyboards, so richer info on modifier keys.
I get that. But I fail to see how that affects SimpleTextInEx behavior or
what the UEFI spec has to say about it.
As I said earlier, the question I am raising is when SimpleTextInEx returns
something like:
Scan Code = 0
Unicode Char = 0x0023 ("#")
Shift Information = 0x8001 (right shift pressed)
is it correct for the editor to reject this as an invalid key?
I say, no, it would be wrong to reject this data because the scan code
is 0 and, therefore, the Unicode character is valid and should be used.
___
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] How to Interpret ReadKeyStrokeEX Data
Dell - Internal Use - Confidential
> From: af...@apple.com [mailto:af...@apple.com]
>
> The big picture difference is the original SimpleTextIn was the least common
> denominator with a serial terminal. The Ex version added more info about
> keyboards, so richer info on modifier keys.
I get that. But I fail to see how that affects SimpleTextInEx behavior or
what the UEFI spec has to say about it.
As I said earlier, the question I am raising is when SimpleTextInEx returns
something like:
Scan Code = 0
Unicode Char = 0x0023 ("#")
Shift Information = 0x8001 (right shift pressed)
is it correct for the editor to reject this as an invalid key?
I say, no, it would be wrong to reject this data because the scan code
is 0 and, therefore, the Unicode character is valid and should be used.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] How to Interpret ReadKeyStrokeEX Data
>Thanks/Ray
>
>> From: jim.dai...@dell.com
>>
>> I guess this is a question of UEFI spec interpretation. In the Console
>> I/O Protocol description of Version 2.5 of the spec (page 456), it says:
>>
>> If the Scan Code is set to 0x00 then the Unicode character is
>> valid and should be used.
>>
>> To me that clearly says it all. The shift modifier is a don't care when
>> the scan code is zero. And, this change in the shell code seems to be a
>> violation of that statement.
>Considering there is the other protocol called SimpleTextIn which only returns
>Scan Code and Unicode Character. The above statement only says that
>consumer should only care one of the fields: Scan Code and Unicode Character.
The editor is not using that protocol, but it matters not because your
question is my point exactly: when the scan code is zero, nothing else
matters except the Unicode character.
>> However, I also see some confusing (and grammatically incorrect) text in
>> the description of the ReadKeyStrokeEx() function of the simple text in
>> protocol that I am guessing is related to this change (*emphasis* mine):
>>
>> When interpreting the data from this function, it should be
>> noted that if a class of printable characters that are normally
>> *adjusted* by shift modifiers (e.g. Shift Key + "f" key) would
>> be presented solely as a KeyData.Key.UnicodeChar without the
>> associated shift state.
>
>Please also considering an implementation of SimpleTextIn, if "SHIFT + 3" is
>pressed, what Unicode Character should be returned since there is no place
>to return the shift state for SimpleTextIn.
>I think it should return "#".
Again the editor does not use that protocol (if it did, I don't think
we'd even be having this discussion!). But to answer your question, of
course it should return "#". There is no question about that. And, when
it returns "#", it would be wrong to ignore that for any reason.
The editor is using the SimpleTextInEx protocol which adds information
about the shift state that SimpleTextIn does not supply.
The question I am raising is when SimpleTextInEx returns, for example:
Scan Code = 0
Unicode Char = 0x0023 ("#")
Shift Information = 0x8001 (right shift pressed)
is it correct for the editor to reject this as an invalid key?
I say, no, it would be wrong to reject this data because the scan code
is 0 and, therefore, the Unicode character is valid and should be used.
The change made back on February 12 says, yes, because the shift data
is greater than 0x8000 (i.e. shift state is valid and some "shift"
key was pressed).
>>> Can you check which keyboard driver are you using?
>>> The keyboard driver is expected to translate "SHIFT" + "3" to "#" (without
>>> Shift state).
>>> I know that some keyboard driver doesn't do that correctly.
>>> E.g.: SHIFT + "3" is translated to "#" but the SHIFT state is not masked
>>> off.
As far as I can tell I am using the standard MdeModulePkg USB driver.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] How to Interpret ReadKeyStrokeEX Data
(Subject changed)
I guess this is a question of UEFI spec interpretation. In the Console
I/O Protocol description of Version 2.5 of the spec (page 456), it says:
If the Scan Code is set to 0x00 then the Unicode character is
valid and should be used.
To me that clearly says it all. The shift modifier is a don't care when
the scan code is zero. And, this change in the shell code seems to be a
violation of that statement.
However, I also see some confusing (and grammatically incorrect) text in
the description of the ReadKeyStrokeEx() function of the simple text in
protocol that I am guessing is related to this change (*emphasis* mine):
When interpreting the data from this function, it should be
noted that if a class of printable characters that are normally
*adjusted* by shift modifiers (e.g. Shift Key + "f" key) would
be presented solely as a KeyData.Key.UnicodeChar without the
associated shift state.
What I think the spec is trying to say here is that for A-Z and a-z,
the consumer does NOT need to look at the shift state to tell "A" from
"a", for example, because the Unicode character will be either "A" or
"a" as appropriate.
I think saying this is unnecessary simply because the earlier statement
(If the Scan Code is set to 0x00 then the Unicode character is valid and
should be used.) covers this case.
Further, I believe this text applies only to the A-Z keys because their
corresponding characters are *adjusted* (to upper case) when the shift
key is pressed. That is, if you adjust "blue" to "BLUE", you have two
different appearances, but only one meaning.
However, a "3" is not *adjusted* to a "#"; they are totally different
characters with different meanings altogether. No C pre-processor would
be happy seeing: "3ifdef SYMBOL".
In any case, I see nothing gained by ignoring keys having a zero scan
code and a valid Unicode character; the spec says that "the Unicode
character is valid and should be used".
Regards,
Jim
> -Original Message-
> From: Ni, Ruiyu [mailto:ruiyu...@intel.com]
> Sent: Thursday, May 31, 2018 7:19 PM
> To: Dailey, Jim
> Cc: Carsey, Jaben; fel...@mail.ru; edk2-devel@lists.01.org
> Subject: RE: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read
> console
>
> Can you check which keyboard driver are you using?
> The keyboard driver is expected to translate "SHIFT" + "3" to "#" (without
> Shift state).
> I know that some keyboard driver doesn't do that correctly.
> E.g.: SHIFT + "3" is translated to "#" but the SHIFT state is not masked off.
>
> [Sorry I thought I sent the mail out days ago]
>
>> -Original Message-
>> From: jim.dai...@dell.com [mailto:jim.dai...@dell.com]
>> Sent: Wednesday, May 23, 2018 3:01 AM
>> To: Ni, Ruiyu
>> Cc: Carsey, Jaben ; fel...@mail.ru; edk2-
>> de...@lists.01.org
>> Subject: RE: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read
>> console
>>
>> Ray,
>>
>> The patch in the message below was applied to the tree on 12 Feb this year
>> (5563281fa2b31093a1cbd415553b9264c5136e89).
>>
>> Part of the change to MainTextEditor.c causes an issue where I cannot enter
>> (at
>> least some) shifted punctuation. For example, after this check in I cannot
>> edit a
>> shell script and create a comment because I cannot enter the "#" character.
>> When I try to type "#", the status bar simply shows "Unknown Command".
>>
>> I don't really understand the change, but if in the MainEditorKeyInput
>> function in
>> file MainTextEditor.c I delete the "NoShiftState" check from the first "else
>> if"
>> below:
>>
>> +//
>> +// dispatch to different components' key handling function
>> +//
>> +if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey(&KeyData)) {
>> + Status = EFI_SUCCESS;
>> +} else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
>> ((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
>> SCAN_PAGE_DOWN {
>> + Status = FileBufferHandleInput (&KeyData.Key);
>> +} else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
>> (KeyData.Key.ScanCode <= SCAN_F12)) {
>> + Status = MenuBarDispatchFunctionKey (&KeyData.Key);
>> +} else {
>> + StatusBarSetStatusString (L"Unknown Command");
>> + FileBufferMouseNeedRefresh = FALSE;
>> +}
>>
>> then I am able to enter "#" and other characters that I previously was
>> unable to
>> enter.
>>
>> Can you have a look at this? I assume any shell binary built with this
>> change will
>> have a similar issue.
>>
>> Thanks,
>> Jim
>>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu
>>> Ni
>>> Sent: Monday, February 12, 2018 9:34 AM
>>> To: edk2-devel@lists.01.org
>>> Cc: Jaben Carsey; Felix
>>> Subject: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read
>>> console
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=682
>>>
>>> Ed
Re: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console
Ray,
The patch in the message below was applied to the tree on 12 Feb this
year (5563281fa2b31093a1cbd415553b9264c5136e89).
Part of the change to MainTextEditor.c causes an issue where I cannot
enter (at least some) shifted punctuation. For example, after this
check in I cannot edit a shell script and create a comment because I
cannot enter the "#" character. When I try to type "#", the status bar
simply shows "Unknown Command".
I don't really understand the change, but if in the MainEditorKeyInput
function in file MainTextEditor.c I delete the "NoShiftState" check
from the first "else if" below:
+//
+// dispatch to different components' key handling function
+//
+if (EFI_NOT_FOUND != MenuBarDispatchControlHotKey(&KeyData)) {
+ Status = EFI_SUCCESS;
+} else if (NoShiftState && ((KeyData.Key.ScanCode == SCAN_NULL) ||
((KeyData.Key.ScanCode >= SCAN_UP) && (KeyData.Key.ScanCode <=
SCAN_PAGE_DOWN {
+ Status = FileBufferHandleInput (&KeyData.Key);
+} else if (NoShiftState && (KeyData.Key.ScanCode >= SCAN_F1) &&
(KeyData.Key.ScanCode <= SCAN_F12)) {
+ Status = MenuBarDispatchFunctionKey (&KeyData.Key);
+} else {
+ StatusBarSetStatusString (L"Unknown Command");
+ FileBufferMouseNeedRefresh = FALSE;
+}
then I am able to enter "#" and other characters that I previously was
unable to enter.
Can you have a look at this? I assume any shell binary built with
this change will have a similar issue.
Thanks,
Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu Ni
Sent: Monday, February 12, 2018 9:34 AM
To: edk2-devel@lists.01.org
Cc: Jaben Carsey; Felix
Subject: [edk2] [PATCH] ShellPkg/[hex]edit: use SimpleTextInEx to read console
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=682
Edit and HexEdit commands assume that SimpleTxtIn translates
Ctrl+ key combinations into Unicode control characters
(0x1-0x1A).
Such translation does not seem to be required by the UEFI spec.
Shell should not rely on implementation specific behavior.
It should instead use SimpleTextInEx to read Ctrl+ key
combinations.
The patch changes edit and hexedit to only consumes SimpleTextInEx.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Reported-by: Felix
Cc: Felix
Cc: Jaben Carsey
---
.../Edit/MainTextEditor.c | 135 +-
.../Edit/TextEditorTypes.h | 21 ++-
.../UefiShellDebug1CommandsLib/EditInputBar.c | 34 +++-
.../UefiShellDebug1CommandsLib/EditInputBar.h | 6 +-
.../UefiShellDebug1CommandsLib/EditMenuBar.c | 38 +++-
.../UefiShellDebug1CommandsLib/EditMenuBar.h | 6 +-
.../HexEdit/HexEditorTypes.h | 25 +--
.../HexEdit/MainHexEditor.c| 205 +
8 files changed, 309 insertions(+), 161 deletions(-)
diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
index 14f51dff19..a197f80a40 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Edit/MainTextEditor.c
@@ -1,7 +1,7 @@
/** @file
Implements editor interface functions.
- Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
+ Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -1362,7 +1362,9 @@ MainCommandDisplayHelp (
{
INT32 CurrentLine;
CHAR16 *InfoString;
- EFI_INPUT_KEY Key;
+ EFI_KEY_DATAKeyData;
+ EFI_STATUS Status;
+ UINTN EventIndex;
//
// print helpInfo
@@ -1371,14 +1373,39 @@ MainCommandDisplayHelp (
InfoString = HiiGetString(gShellDebug1HiiHandle,
MainMenuHelpInfo[CurrentLine], NULL);
ShellPrintEx (0, CurrentLine+1, L"%E%s%N", InfoString);
}
-
+
//
// scan for ctrl+w
//
- do {
-gST->ConIn->ReadKeyStroke (gST->ConIn, &Key);
- } while(SCAN_CONTROL_W != Key.UnicodeChar);
+ while (TRUE) {
+Status = gBS->WaitForEvent (1, &MainEditor.TextInputEx->WaitForKeyEx,
&EventIndex);
+if (EFI_ERROR (Status) || (EventIndex != 0)) {
+ continue;
+}
+Status = MainEditor.TextInputEx->ReadKeyStrokeEx (MainEditor.TextInputEx,
&KeyData);
+if (EFI_ERROR (Status)) {
+ continue;
+}
+if ((KeyData.KeyState.KeyShiftState & EFI_SHIFT_STATE_VALID) == 0) {
+ //
+ // For consoles that don't support shift state reporting,
+ // CTRL+W is translated to L'W' - L'A' + 1.
+ //
+ if (KeyData.Key.UnicodeChar == L'W' - L'A' + 1) {
+break;
+ }
+
Re: [edk2] how to create environmental variable for Shell
Please excuse the stupid email plug-in I have to use that puts the "Internal Use" line in my text emails. The plug-in does not allow me to see that text, so I have to remember to remove it every time even though I never see it. Sadly, I sometimes forget. -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dailey, Jim Sent: Friday, November 3, 2017 6:49 AM To: tiger...@zhaoxin.com Cc: edk2-devel@lists.01.org Subject: Re: [edk2] how to create environmental variable for Shell Dell - Internal Use - Confidential <=== No, this is *NOT* true! You need to use the shell's GUID, gShellVariableGuid, if you want to create a shell variable. Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Tiger Liu Sent: Friday, November 3, 2017 12:01 AM To: edk2-devel@lists.01.org Subject: [edk2] how to create environmental variable for Shell Hi, experts: I have a question about creating shell’s environmental variable. Such as : StartupDelay I tried to create this variable at uefi boot phase. …… TmpVal = 2; Status = gRT->SetVariable ( L"StartupDelay", &gEfiGlobalVariableGuid, EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS |EFI_VARIABLE_NON_VOLATILE, sizeof (TmpVal), &TmpVal ); …… When boot to shell, it still delays 5 seconds. I tried to use shell cmd ‘set’, then it’s ok, shell’s default delay switched to 2 seconds. I used dmpstore cmd to list current system’s variables, and found 2 StartupDelay variables: Efi:StartupDelay SEnv:StartupDelay So, my question is: How to create “SEnv:StartupDelay” variable in UEFI BIOS code. Thanks Best wishes, 保密声明： 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。 CONFIDENTIAL NOTE: This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited. ___ 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] how to create environmental variable for Shell
Dell - Internal Use - Confidential You need to use the shell's GUID, gShellVariableGuid, if you want to create a shell variable. Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Tiger Liu Sent: Friday, November 3, 2017 12:01 AM To: edk2-devel@lists.01.org Subject: [edk2] how to create environmental variable for Shell Hi, experts: I have a question about creating shell’s environmental variable. Such as : StartupDelay I tried to create this variable at uefi boot phase. …… TmpVal = 2; Status = gRT->SetVariable ( L"StartupDelay", &gEfiGlobalVariableGuid, EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS |EFI_VARIABLE_NON_VOLATILE, sizeof (TmpVal), &TmpVal ); …… When boot to shell, it still delays 5 seconds. I tried to use shell cmd ‘set’, then it’s ok, shell’s default delay switched to 2 seconds. I used dmpstore cmd to list current system’s variables, and found 2 StartupDelay variables: Efi:StartupDelay SEnv:StartupDelay So, my question is: How to create “SEnv:StartupDelay” variable in UEFI BIOS code. Thanks Best wishes, 保密声明： 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其内容做任何未经授权的查阅、使用、复制或转发。 CONFIDENTIAL NOTE: This email contains confidential or legally privileged information and is for the sole use of its intended recipient. Any unauthorized review, use, copying or forwarding of this email or the content of this email is strictly prohibited. ___ 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 2/3] ShellPkg: Fix misuses of AllocateCopyPool
Isn't ReallocatePool is the correct function to use in these cases?
For example:
NewCommandLine1 = ReallocatePool(NewSize, StrSize(OriginalCommandLine),
OriginalCommandLine;
Regards,
Jim
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ni, Ruiyu
Sent: Friday, November 3, 2017 3:23 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Carsey, Jaben ; Bi, Dandan
Subject: Re: [edk2] [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool
2 comments below.
-Original Message-
From: Wang, Jian J
Sent: Friday, November 3, 2017 12:58 PM
To: edk2-devel@lists.01.org
Cc: Carsey, Jaben ; Ni, Ruiyu ; Bi,
Dandan
Subject: [PATCH 2/3] ShellPkg: Fix misuses of AllocateCopyPool
AllocateCopyPool(AllocationSize, *Buffer) will copy "AllocationSize" bytes of
memory from old "Buffer" to new allocated one. If "AllocationSize" is bigger
than size of "Buffer", heap memory overflow occurs during copy.
The solution is to allocate pool first then copy the necessary bytes to new
memory. This can avoid copying extra bytes from unknown memory range.
Cc: Jaben Carsey
Cc: Ruiyu Ni
Cc: Bi Dandan
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
ShellPkg/Application/Shell/Shell.c | 4 +++-
ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c | 6 --
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index 5471930ba1..24a04ca323 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1646,7 +1646,9 @@ ShellConvertVariables (
//
// now do the replacements...
//
- NewCommandLine1 = AllocateCopyPool(NewSize, OriginalCommandLine);
+ NewCommandLine1 = AllocatePool(NewSize);
+ ASSERT (NewCommandLine1 != NULL);
[Ray] 1. Please do not use assertion because there is NULL check in the below
if-statement.
The rule in ShellPkg is avoid using assertion.
+ CopyMem (NewCommandLine1, OriginalCommandLine, StrSize
(OriginalCommandLine));
NewCommandLine2 = AllocateZeroPool(NewSize);
ItemTemp= AllocateZeroPool(ItemSize+(2*sizeof(CHAR16)));
if (NewCommandLine1 == NULL || NewCommandLine2 == NULL || ItemTemp == NULL) {
diff --git a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
index 1122c89b8b..5de62219b3 100644
--- a/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
+++ b/ShellPkg/Library/UefiShellBcfgCommandLib/UefiShellBcfgCommandLib.c
@@ -143,10 +143,11 @@ UpdateOptionalData(
OriginalOptionDataSize += (*(UINT16*)(OriginalData + sizeof(UINT32)));
OriginalOptionDataSize -= OriginalSize;
NewSize = OriginalSize - OriginalOptionDataSize + DataSize;
-NewData = AllocateCopyPool(NewSize, OriginalData);
+NewData = AllocatePool(NewSize);
if (NewData == NULL) {
Status = EFI_OUT_OF_RESOURCES;
} else {
+ CopyMem (NewData, OriginalData, OriginalSize - OriginalOptionDataSize);
CopyMem(NewData + OriginalSize - OriginalOptionDataSize, Data, DataSize);
}
}
@@ -1120,11 +1121,12 @@ BcfgAddOpt(
// Now we know how many EFI_INPUT_KEY structs we need to attach to the
end of the EFI_KEY_OPTION struct.
// Re-allocate with the added information.
//
-KeyOptionBuffer = AllocateCopyPool(sizeof(EFI_KEY_OPTION) +
(sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount),
&NewKeyOption);
+KeyOptionBuffer = AllocatePool (sizeof(EFI_KEY_OPTION) +
(sizeof(EFI_INPUT_KEY) * NewKeyOption.KeyData.Options.InputKeyCount));
if (KeyOptionBuffer == NULL) {
ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_MEM),
gShellBcfgHiiHandle, L"bcfg");
ShellStatus = SHELL_OUT_OF_RESOURCES;
}
[Ray] 2. Should the above NULL check return?
+CopyMem (KeyOptionBuffer, &NewKeyOption, sizeof(EFI_KEY_OPTION));
}
for (LoopCounter = 0 ; ShellStatus == SHELL_SUCCESS && LoopCounter <
NewKeyOption.KeyData.Options.InputKeyCount; LoopCounter++) {
//
--
2.14.1.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
[edk2] Shell Non-conformity to the Spec
The shell spec says that "Each environment variable has a case-sensitive name ...". In the EfiShellSetEnv function of ShellProtocol.c a case-insensitive compare is performed against the variable that is to be set to see if it is one of the read-only variables. That means one cannot set a variable named, for example, CWD, even though "cwd" and "CWD" are two different variable names according to the spec. Should this be changed to a case-sensitive comparison? Regards, Jim ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Shell input redirection question
I think the technicality here is that given "pci < inputfile", no parameters are passed to the pci command, so it should, according to the spec, display all the devices. Another problem may be that in the "pci < inputfile" case, the pci command has no way to know that its input is being redirected and that it should behave differently than normal. I may be wrong about that point; I know it is possible, but it might require some sort of generic change or addition to the shell's behavior. A different tack would be to add (in a future spec version) a command line argument that simply indicated to pci that its input (commands) are in some file (e.g. "pci -cmdfile inputfile"). Of course, if that were to happen, the new spec should also specify the format of the command file's content. A simple way to do what Tiger was trying and that works right now is to create a shell script instead of an input file: @echo -off pci 00 00 01 -i pci 00 00 02 -i Regards, Jim -Original Message- From: Carsey, Jaben [mailto:jaben.car...@intel.com] Sent: Thursday, September 21, 2017 8:11 AM To: Dailey, Jim ; tiger...@zhaoxin.com Cc: edk2-devel@lists.01.org Subject: RE: [edk2] Shell input redirection question If the requirement for PCI to display the information when there are no parameters present, I see no reason that it could not also use a file for input. i.e. "pci" must act according to the spec, but "pci < inputfile" is not prohibited. I think that as no code currently uses PCI with file input as long as current behavior still works it should be fine. I would think that would be a great improvement for the PCI command. -Jaben > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > jim.dai...@dell.com > Sent: Thursday, September 21, 2017 4:52 AM > To: tiger...@zhaoxin.com > Cc: edk2-devel@lists.01.org > Subject: Re: [edk2] Shell input redirection question > Importance: High > > The shell's pci command was not written to read from standard input. It > expects all its input on the command line. > > I would say in general that if you execute a command and pass it no > parameters, and it then prompts you in some way for input, then that > command will likely accept input redirected from a file. > > If you execute pci without any parameters, it simply lists all the > devices in the system and terminates, so it clearly is not prepared to > read from standard input (or a redirected file). > > It is strictly up to whomever writes a program/command whether they do > so in a manner that allows it to accept input from standard input. For > example, I have written a grep utility for the shell that expects one or > more filenames to search to be on the command line; however, if there > are no filenames on the command line, the utility searches standard > input. > > Regards, > Jim > > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Tiger Liu > Sent: Thursday, September 21, 2017 1:11 AM > To: edk2-devel@lists.01.org > Subject: [edk2] Shell input redirection question > > Hi, experts: > I have a question about input redirection in Shell environment. > > Take pci command as sample. > I wrote a txt file(file name is : inputsample.txt), its content is: > 00 00 01 -i > 00 00 02 –i > > It means I just wanted to dump D0F1/D0F2’s config space. > > Then, I use this command sequence in shell : > pci > But it seems not recognize this input file’s content. > > Why? > > Thanks > > best wishes, > > > 保密声明： > 本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件 > 或其内容做任何未经授权的查阅、使用、复制或转发。 > CONFIDENTIAL NOTE: > This email contains confidential or legally privileged information and is for > the > sole use of its intended recipient. Any unauthorized review, use, copying or > forwarding of this email or the content of this email is strictly prohibited. > ___ > 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] Shell input redirection question
The shell's pci command was not written to read from standard input. It expects all its input on the command line. I would say in general that if you execute a command and pass it no parameters, and it then prompts you in some way for input, then that command will likely accept input redirected from a file. If you execute pci without any parameters, it simply lists all the devices in the system and terminates, so it clearly is not prepared to read from standard input (or a redirected file). It is strictly up to whomever writes a program/command whether they do so in a manner that allows it to accept input from standard input. For example, I have written a grep utility for the shell that expects one or more filenames to search to be on the command line; however, if there are no filenames on the command line, the utility searches standard input. Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Tiger Liu Sent: Thursday, September 21, 2017 1:11 AM To: edk2-devel@lists.01.org Subject: [edk2] Shell input redirection question Hi, experts: I have a question about input redirection in Shell environment. Take pci command as sample. I wrote a txt file(file name is : inputsample.txt), its content is: 00 00 01 -i 00 00 02 –i It means I just wanted to dump D0F1/D0F2’s config space. Then, I use this command sequence in shell : pci 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
[edk2] Coding Style Line Length
Laszlo, Are there any special considerations for Print() statements with respect to the 80 character line length? If "No", what is the preferred format if one wants to print strings that are 80 or more characters such as: Print(L"123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789\n"); Regards, Jim ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SMBios configuration table not present until late stage of boot
It is a tricky problem. What I would like is for a new protocol to be defined, which should not rely on devices, to contain certain identifying information like this that would be useful to device drivers. It could be created early in DXE. What I fear is some future requirement that SMBIOS be made available at some definitive time pre-OS boot. You may have to get support from the BIOS vendor. If you are doing a driver for a particular system, that might not be too bad of a solution; but if you're trying to develop some generic driver, I don't have a good suggestion. Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Tomas Pilar (tpilar) Sent: Thursday, May 18, 2017 10:14 AM To: Dailey, Jim Cc: edk2-devel@lists.01.org Subject: Re: [edk2] SMBios configuration table not present until late stage of boot This does make sense. Do you have a suggestion how I would go about finding/creating a unique identifier for the system during preboot? Cheers, Tom On 18/05/17 16:11, jim.dai...@dell.com wrote: > Not a helpful comment, but I wanted to air my feelings on the topic: > > I view SMBIOS as data strictly for OS-level consumption and not for > any pre-boot code. I'm sure I'm in the minority, however. > > One of the problems is that the BIOS needs to have scanned all > devices/resources and perhaps executed a connect all before the > tables can be generated (or at least completed). > > Regards, > Jim > > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Tomas > Pilar (tpilar) > Sent: Thursday, May 18, 2017 10:01 AM > To: edk2-devel@lists.01.org > Subject: [edk2] SMBios configuration table not present until late stage of > boot > > Hi, > > I am trying to read the system UUID from the System Table (Type 1) in > the SMBios set of tables. I am doing this during DriverBinding.Start() > part of the UEFI_DRIVER initialisation. Unfortunately the > gST->ConfigurationTable only contains 6 tables and SMBios is not one of > them. > > Once I boot into UEFI shell or start a PXE booting process, the > gST->ConfigurationTable now contains 8 tables and SMBios is one of the > two new tables. If I however only boot to a HDD, this never seems to > happen. > > Can someone offer some insight why this might be so and how would I go > about forcing the platform to provide the SMBios in > gST->ConfigurationTable at a sensible point? > > Incidentally it seems ExitBootServices is not signaled on this platform > if the boot goes through to HDD either, which is another strange thing ... > > Cheers, > Tom > > > > ___ > 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] SMBios configuration table not present until late stage of boot
Not a helpful comment, but I wanted to air my feelings on the topic: I view SMBIOS as data strictly for OS-level consumption and not for any pre-boot code. I'm sure I'm in the minority, however. One of the problems is that the BIOS needs to have scanned all devices/resources and perhaps executed a connect all before the tables can be generated (or at least completed). Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Tomas Pilar (tpilar) Sent: Thursday, May 18, 2017 10:01 AM To: edk2-devel@lists.01.org Subject: [edk2] SMBios configuration table not present until late stage of boot Hi, I am trying to read the system UUID from the System Table (Type 1) in the SMBios set of tables. I am doing this during DriverBinding.Start() part of the UEFI_DRIVER initialisation. Unfortunately the gST->ConfigurationTable only contains 6 tables and SMBios is not one of them. Once I boot into UEFI shell or start a PXE booting process, the gST->ConfigurationTable now contains 8 tables and SMBios is one of the two new tables. If I however only boot to a HDD, this never seems to happen. Can someone offer some insight why this might be so and how would I go about forcing the platform to provide the SMBios in gST->ConfigurationTable at a sensible point? Incidentally it seems ExitBootServices is not signaled on this platform if the boot goes through to HDD either, which is another strange thing ... Cheers, Tom ___ 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
[edk2] Possible UEFI Shell DrvDiag Command Error
Although this question is about the shell's drvdiag command, the answer is related to the interpretation of the UEFI specification with respect to the diagnostics protocol's RunDiagnostics() function. Ultimately, the question is, is the drvdiag command in error in trying to free the ErrorType pointer, or is a driver vendor in error when they return an ErrorType parameter that does not point to memory that was allocated using gBS->AllocatePool? The diagnostics protocol's RunDiagnostics() function is partially described in the UEFI spec like this: typedef EFI_STATUS (EFIAPI *EFI_DRIVER_DIAGNOSTICS2_RUN_DIAGNOSTICS) ( IN EFI_DRIVER_DIAGNOSTICS2_PROTOCOL *This, ... OUT EFI_GUID**ErrorType, OUT UINTN *BufferSize, OUT CHAR16 **Buffer ); ... ErrorType A GUID that defines the format of the data returned in Buffer. BufferSize The size, in bytes, of the data returned in Buffer. Buffer A buffer that contains a Null-terminated string plus some additional data whose format is defined by ErrorType. Buffer is allocated by this function with EFI_BOOT_SERVICES.AllocatePool(), and it is the caller's responsibility to free it with a call to EFI_BOOT_SERVICES.FreePool(). After a call to RunDiagnostics, the drvdiag command attempts to free the ErrorType pointer if it is non-NULL, yet nothing in the UEFI spec says that this is proper. 123456789012345678901234567890123456789012345678901234567890123456789012 The UEFI spec goes to great length to point out that *Buffer* must be allocated by the callee using gBS->AllocatePool and that the caller must use gBS->FreePool to release the memory once it is no longer needed. However, there is no such language with respect to ErrorType. So, repeating the question, is the drvdiag command in error in trying to free the ErrorType pointer, or is a driver vendor in error when they return an ErrorType parameter that does not point to memory that was allocated using gBS->AllocatePool? Regards, Jim ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] UEFI Shell Lib Constructor and Shell Parameters Protocol
Thanks, Jaben. That makes sense.
Now I see the real issue is with the "app" I ran across. It is a driver,
and it was trying to access command line args. The driver crashed when
it tried to access the command line, but it had been loaded in memory via
the "load" command.
So, it seems "load" has no mechanism to pass command line arguments to a
driver, and as a result, it apparently doesn't load the shell parameters
protocol on the driver's handle, which caused the crash.
Regards,
Jim
-Original Message-
From: Carsey, Jaben [mailto:jaben.car...@intel.com]
Sent: Wednesday, April 5, 2017 10:08 AM
To: Dailey, Jim ; Ni, Ruiyu
Cc: edk2-devel@lists.01.org
Subject: RE: UEFI Shell Lib Constructor and Shell Parameters Protocol
Jim,
That protocol must be installed on your applications own image handle for it to
be valid. Locating the protocol on some other image would result with finding
the other image's command line parameters and the like...
-Jaben
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> jim.dai...@dell.com
> Sent: Wednesday, April 05, 2017 7:07 AM
> To: Carsey, Jaben ; Ni, Ruiyu
> Cc: edk2-devel@lists.01.org
> Subject: [edk2] UEFI Shell Lib Constructor and Shell Parameters Protocol
> Importance: High
>
>
> A question or two for all shell experts:
>
> In the UEFI Shell Lib constructor code, if the Shell protocol cannot
> be opened, then an attempt is made to locate it before "giving up".
>
> However, if the Shell parameters protocol cannot be opened, no attempt
> is made to locate it---the code simply leaves the parameters protocol
> pointer set to NULL. Can anyone explain why this is?
>
> I came across an app that crashes because of this behavior, but if I
> add code to try and locate the parameters protocol, then the app works
> as designed WRT accessing command line parameters. Is there some
> lurking issue if an attempt to locate the parameters protocol is made
> and is successful?
>
> Thanks,
> Jim
>
> ---
> Here is the relevant code:
>
> EFI_STATUS
> ShellLibConstructorWorker (
> IN EFI_HANDLEImageHandle,
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> EFI_STATUS Status;
>
> //
> // UEFI 2.0 shell interfaces (used preferentially)
> //
> Status = gBS->OpenProtocol(
> ImageHandle,
> &gEfiShellProtocolGuid,
> (VOID **)&gEfiShellProtocol,
> ImageHandle,
> NULL,
> EFI_OPEN_PROTOCOL_GET_PROTOCOL
>);
> if (EFI_ERROR(Status)) {
> //
> // Search for the shell protocol
> //
> Status = gBS->LocateProtocol(
> &gEfiShellProtocolGuid,
> NULL,
> (VOID **)&gEfiShellProtocol
> );
> if (EFI_ERROR(Status)) {
> gEfiShellProtocol = NULL;
> }
> }
> Status = gBS->OpenProtocol(
> ImageHandle,
> &gEfiShellParametersProtocolGuid,
> (VOID **)&gEfiShellParametersProtocol,
> ImageHandle,
> NULL,
> EFI_OPEN_PROTOCOL_GET_PROTOCOL
>);
> if (EFI_ERROR(Status)) {
> #if 1 // _Dell_ : Search for the parameters protocol too!
> //
> // Search for the shell parameters protocol
> //
> Status = gBS->LocateProtocol(
> &gEfiShellParametersProtocolGuid,
> NULL,
> (VOID **)&gEfiShellParametersProtocol
> );
> if (EFI_ERROR(Status)) {
> gEfiShellParametersProtocol = NULL;
> }
> #else
> gEfiShellParametersProtocol = NULL;
> #endif
> }
> ...
> ___
> 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
[edk2] UEFI Shell Lib Constructor and Shell Parameters Protocol
A question or two for all shell experts:
In the UEFI Shell Lib constructor code, if the Shell protocol cannot
be opened, then an attempt is made to locate it before "giving up".
However, if the Shell parameters protocol cannot be opened, no attempt
is made to locate it---the code simply leaves the parameters protocol
pointer set to NULL. Can anyone explain why this is?
I came across an app that crashes because of this behavior, but if I
add code to try and locate the parameters protocol, then the app works
as designed WRT accessing command line parameters. Is there some
lurking issue if an attempt to locate the parameters protocol is made
and is successful?
Thanks,
Jim
---
Here is the relevant code:
EFI_STATUS
ShellLibConstructorWorker (
IN EFI_HANDLEImageHandle,
IN EFI_SYSTEM_TABLE *SystemTable
)
{
EFI_STATUS Status;
//
// UEFI 2.0 shell interfaces (used preferentially)
//
Status = gBS->OpenProtocol(
ImageHandle,
&gEfiShellProtocolGuid,
(VOID **)&gEfiShellProtocol,
ImageHandle,
NULL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
if (EFI_ERROR(Status)) {
//
// Search for the shell protocol
//
Status = gBS->LocateProtocol(
&gEfiShellProtocolGuid,
NULL,
(VOID **)&gEfiShellProtocol
);
if (EFI_ERROR(Status)) {
gEfiShellProtocol = NULL;
}
}
Status = gBS->OpenProtocol(
ImageHandle,
&gEfiShellParametersProtocolGuid,
(VOID **)&gEfiShellParametersProtocol,
ImageHandle,
NULL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
if (EFI_ERROR(Status)) {
#if 1 // _Dell_ : Search for the parameters protocol too!
//
// Search for the shell parameters protocol
//
Status = gBS->LocateProtocol(
&gEfiShellParametersProtocolGuid,
NULL,
(VOID **)&gEfiShellParametersProtocol
);
if (EFI_ERROR(Status)) {
gEfiShellParametersProtocol = NULL;
}
#else
gEfiShellParametersProtocol = NULL;
#endif
}
...
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage
Dell - Internal Use - Confidential Thanks, Jeff. I wasn't thinking that through properly. Printing the address and size seems ok. Then one could use dmem, for example, to dump the actual data. -Original Message- From: Jeff Westfahl [mailto:jeff.westf...@ni.com] Sent: Friday, February 17, 2017 11:44 AM To: Carsey, Jaben Cc: Dailey, Jim ; Ni, Ruiyu ; edk2-devel@lists.01.org; jeff.westf...@ni.com Subject: RE: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage Using "%s" tries to print the data as text, but crashes if the pointer to that data is garbage. Using "%x" just prints the value of the pointer, whether it's valid or not. It doesn't print any of the actual data. On Fri, 17 Feb 2017, Carsey, Jaben wrote: > I think that the long term solution may improve upon this one, but if the > data is hex, then printing in hex at all would seem to be the logical first > step. > >> -Original Message- >> From: jim.dai...@dell.com [mailto:jim.dai...@dell.com] >> Sent: Friday, February 17, 2017 9:36 AM >> To: Carsey, Jaben >> Cc: Ni, Ruiyu ; edk2-devel@lists.01.org; >> jeff.westf...@ni.com >> Subject: RE: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format >> specifier for LoadedImage >> Importance: High >> >> My point was that printing one byte of the data as hex is not much better (as >> far >> as avoiding the crash is concerned) than simply not printing the data at >> all. If >> we are just trying to avoid the crash, printing nothing is even simpler. >> >> If the correct representation is hex bytes, fine; but why print only 1 of >> them >> and not all of them? >> >> >> -Original Message- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Carsey, Jaben >> Sent: Friday, February 17, 2017 11:28 AM >> To: Jeff Westfahl >> Cc: Ni, Ruiyu ; Carsey, Jaben >> ; edk2-devel@lists.01.org >> Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format >> specifier for LoadedImage >> >> I was reading the email. I was also waiting and making sure there was >> consensus since I didn't have a strong opinion. I will let Ray check also, >> but I >> think the fix is good. >> >> Reviewed-by: Jaben Carsey >> >>> -Original Message- >>> From: Jeff Westfahl [mailto:jeff.westf...@ni.com] >>> Sent: Friday, February 17, 2017 8:55 AM >>> To: Jeff Westfahl >>> Cc: edk2-devel@lists.01.org; Carsey, Jaben ; Ni, >>> Ruiyu >>> Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format >>> specifier for LoadedImage >>> Importance: High >>> >>> Jaben, Ruiyu, >>> >>> Sorry, I forgot to cc you as the ShellPkg maintainers when I submitted >>> this patch. >>> >>> Regards, >>> Jeff Westfahl >>> >>> On Wed, 15 Feb 2017, Jeff Westfahl wrote: >>> Jim, I agree that those are good ideas. However, such an implementation >> would still crash on a BIOS built against the EDK II before commit 891d844. I think it might be best to resolve the crash with the simple patch I have made, >>> and defer your suggestions for now. Regards, Jeff On Tue, 14 Feb 2017, jim.dai...@dell.com wrote: > Please disregard the earlier "Confidential" text. The stupid plug-in that > adds this > does not show the text in the mail when it is composed in text mode, so >> I > often > forget to turn this "feature" off when posting. Sorry. > > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf >> Of > Dailey, Jim > Sent: Tuesday, February 14, 2017 4:40 PM > To: jeff.westf...@ni.com; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format > specifier for LoadedImage > > Jeff, > > Perhaps a better approach is to print *all* the LoadOptions data as hex > bytes? > > In addition, one might first analyze the LoadOptions data, and, when > apropos, > print obvious strings as strings? > > Regards, > Jim > > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf >> Of >>> Jeff > Westfahl > Sent: Tuesday, February 14, 2017 3:54 PM > To: edk2-devel@lists.01.org > Subject: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format >>> specifier > for LoadedImage > > The format specifier for the LoadOptions field of the LoadedImage >>> protocol > is "%s". However, the data in LoadOptions is often generic binary data. A > format specifier of "%x" is more appropriate for this field. > > Using "dh -v" with format specifier "%s" on BIOS images based on EDK II > source before commit 891d844 can cause a crash. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Westfahl > --- > ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +- > 1 file changed, 1 insertion(+
Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage
My point was that printing one byte of the data as hex is not much better (as far as avoiding the crash is concerned) than simply not printing the data at all. If we are just trying to avoid the crash, printing nothing is even simpler. If the correct representation is hex bytes, fine; but why print only 1 of them and not all of them? -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Carsey, Jaben Sent: Friday, February 17, 2017 11:28 AM To: Jeff Westfahl Cc: Ni, Ruiyu ; Carsey, Jaben ; edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage I was reading the email. I was also waiting and making sure there was consensus since I didn't have a strong opinion. I will let Ray check also, but I think the fix is good. Reviewed-by: Jaben Carsey > -Original Message- > From: Jeff Westfahl [mailto:jeff.westf...@ni.com] > Sent: Friday, February 17, 2017 8:55 AM > To: Jeff Westfahl > Cc: edk2-devel@lists.01.org; Carsey, Jaben ; Ni, > Ruiyu > Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format > specifier for LoadedImage > Importance: High > > Jaben, Ruiyu, > > Sorry, I forgot to cc you as the ShellPkg maintainers when I submitted > this patch. > > Regards, > Jeff Westfahl > > On Wed, 15 Feb 2017, Jeff Westfahl wrote: > > > Jim, > > > > I agree that those are good ideas. However, such an implementation would > > still crash on a BIOS built against the EDK II before commit 891d844. I > > think > > it might be best to resolve the crash with the simple patch I have made, > and > > defer your suggestions for now. > > > > Regards, > > Jeff > > > > On Tue, 14 Feb 2017, jim.dai...@dell.com wrote: > > > >> Please disregard the earlier "Confidential" text. The stupid plug-in that > >> adds this > >> does not show the text in the mail when it is composed in text mode, so I > >> often > >> forget to turn this "feature" off when posting. Sorry. > >> > >> -Original Message- > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > >> Dailey, Jim > >> Sent: Tuesday, February 14, 2017 4:40 PM > >> To: jeff.westf...@ni.com; edk2-devel@lists.01.org > >> Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format > >> specifier for LoadedImage > >> > >> Jeff, > >> > >> Perhaps a better approach is to print *all* the LoadOptions data as hex > >> bytes? > >> > >> In addition, one might first analyze the LoadOptions data, and, when > >> apropos, > >> print obvious strings as strings? > >> > >> Regards, > >> Jim > >> > >> -Original Message- > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Jeff > >> Westfahl > >> Sent: Tuesday, February 14, 2017 3:54 PM > >> To: edk2-devel@lists.01.org > >> Subject: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format > specifier > >> for LoadedImage > >> > >> The format specifier for the LoadOptions field of the LoadedImage > protocol > >> is "%s". However, the data in LoadOptions is often generic binary data. A > >> format specifier of "%x" is more appropriate for this field. > >> > >> Using "dh -v" with format specifier "%s" on BIOS images based on EDK II > >> source before commit 891d844 can cause a crash. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Jeff Westfahl > >> --- > >> ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git > a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni > >> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni > >> index 0d51627c5f..273a4201bc 100644 > >> --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni > >> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni > >> @@ -354,7 +354,7 @@ > >> " DeviceHandle..: > >> %%H%x%%N\r\n" > >> " FilePath..: > >> %%H%x%%N\r\n" > >> " OptionsSize...: > >> %%H%x%%N\r\n" > >> - " LoadOptions...: > >> %%H%s%%N\r\n" > >> + " LoadOptions...: > >> %%H%x%%N\r\n" > >> " ImageBase.: > >> %%H%x%%N\r\n" > >> " ImageSize.: > >> %%H%Lx%%N\r\n" > >> " CodeType..: > >> %%H%s%%N\r\n" > >> -- > >> 2.11.0.windows.3 > >> > >> ___ > >> 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 >
Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage
Please disregard the earlier "Confidential" text. The stupid plug-in that adds this does not show the text in the mail when it is composed in text mode, so I often forget to turn this "feature" off when posting. Sorry. -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Dailey, Jim Sent: Tuesday, February 14, 2017 4:40 PM To: jeff.westf...@ni.com; edk2-devel@lists.01.org Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage Jeff, Perhaps a better approach is to print *all* the LoadOptions data as hex bytes? In addition, one might first analyze the LoadOptions data, and, when apropos, print obvious strings as strings? Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jeff Westfahl Sent: Tuesday, February 14, 2017 3:54 PM To: edk2-devel@lists.01.org Subject: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage The format specifier for the LoadOptions field of the LoadedImage protocol is "%s". However, the data in LoadOptions is often generic binary data. A format specifier of "%x" is more appropriate for this field. Using "dh -v" with format specifier "%s" on BIOS images based on EDK II source before commit 891d844 can cause a crash. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Westfahl --- ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni index 0d51627c5f..273a4201bc 100644 --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni @@ -354,7 +354,7 @@ " DeviceHandle..: %%H%x%%N\r\n" " FilePath..: %%H%x%%N\r\n" " OptionsSize...: %%H%x%%N\r\n" - " LoadOptions...: %%H%s%%N\r\n" + " LoadOptions...: %%H%x%%N\r\n" " ImageBase.: %%H%x%%N\r\n" " ImageSize.: %%H%Lx%%N\r\n" " CodeType..: %%H%s%%N\r\n" -- 2.11.0.windows.3 ___ 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 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage
Dell - Internal Use - Confidential Jeff, Perhaps a better approach is to print *all* the LoadOptions data as hex bytes? In addition, one might first analyze the LoadOptions data, and, when apropos, print obvious strings as strings? Regards, Jim -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jeff Westfahl Sent: Tuesday, February 14, 2017 3:54 PM To: edk2-devel@lists.01.org Subject: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage The format specifier for the LoadOptions field of the LoadedImage protocol is "%s". However, the data in LoadOptions is often generic binary data. A format specifier of "%x" is more appropriate for this field. Using "dh -v" with format specifier "%s" on BIOS images based on EDK II source before commit 891d844 can cause a crash. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Westfahl --- ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni index 0d51627c5f..273a4201bc 100644 --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni @@ -354,7 +354,7 @@ " DeviceHandle..: %%H%x%%N\r\n" " FilePath..: %%H%x%%N\r\n" " OptionsSize...: %%H%x%%N\r\n" - " LoadOptions...: %%H%s%%N\r\n" + " LoadOptions...: %%H%x%%N\r\n" " ImageBase.: %%H%x%%N\r\n" " ImageSize.: %%H%Lx%%N\r\n" " CodeType..: %%H%s%%N\r\n" -- 2.11.0.windows.3 ___ 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
