From: "Carsey, Jaben" <jaben.carsey@intel.com>
To: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Chen, Chen A" <chen.a.chen@intel.com>,
"Carsey, Jaben" <jaben.carsey@intel.com>
Subject: Re: [PATCH] ShellPkg/Application: Fix ">v" cannot update environment variable
Date: Thu, 8 Dec 2016 21:59:03 +0000 [thread overview]
Message-ID: <CB6E33457884FA40993F35157061515C54B0EA7B@FMSMSX103.amr.corp.intel.com> (raw)
In-Reply-To: <20161208014519.281120-1-ruiyu.ni@intel.com>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, December 7, 2016 5:45 PM
> To: edk2-devel@lists.01.org
> Cc: Chen, Chen A <chen.a.chen@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>
> Subject: [PATCH] ShellPkg/Application: Fix ">v" cannot update environment
> variable
> Importance: High
>
> From: Chen A Chen <chen.a.chen@intel.com>
>
> When ">v" is used to redirect the command output to environment variable
> (e.g.: "echo xxx >v yyy"), we only called SetVariable() to update the variable
> storage but forgot to update the cached environment variables in
> gShellEnvVarList.
> When updating the variable storage, the existing code unnecessary saved the
> ending NULL character into variable storage.
>
> The patch fixes all the above issues.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chen A Chen <chen.a.chen@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> ---
> ShellPkg/Application/Shell/FileHandleWrappers.c | 170 ++++++++++++++++++---
> ---
> ShellPkg/Application/Shell/ShellEnvVar.c | 12 +-
> ShellPkg/Application/Shell/ShellProtocol.c | 4 +-
> 3 files changed, 142 insertions(+), 44 deletions(-)
>
> diff --git a/ShellPkg/Application/Shell/FileHandleWrappers.c
> b/ShellPkg/Application/Shell/FileHandleWrappers.c
> index 18f4169..3c11d82 100644
> --- a/ShellPkg/Application/Shell/FileHandleWrappers.c
> +++ b/ShellPkg/Application/Shell/FileHandleWrappers.c
> @@ -1040,6 +1040,7 @@ FileInterfaceEnvClose(
> UINTN NewSize;
> EFI_STATUS Status;
> BOOLEAN Volatile;
> + UINTN TotalSize;
>
> //
> // Most if not all UEFI commands will have an '\r\n' at the end of any output.
> @@ -1049,6 +1050,7 @@ FileInterfaceEnvClose(
> //
> NewBuffer = NULL;
> NewSize = 0;
> + TotalSize = 0;
>
> Status = IsVolatileEnv (((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> &Volatile);
> if (EFI_ERROR (Status)) {
> @@ -1057,7 +1059,8 @@ FileInterfaceEnvClose(
>
> Status =
> SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT*
> )This)->Name, &NewSize, NewBuffer);
> if (Status == EFI_BUFFER_TOO_SMALL) {
> - NewBuffer = AllocateZeroPool(NewSize + sizeof(CHAR16));
> + TotalSize = NewSize + sizeof (CHAR16);
> + NewBuffer = AllocateZeroPool (TotalSize);
> if (NewBuffer == NULL) {
> return EFI_OUT_OF_RESOURCES;
> }
> @@ -1066,17 +1069,43 @@ FileInterfaceEnvClose(
>
> if (!EFI_ERROR(Status) && NewBuffer != NULL) {
>
> - if (StrSize(NewBuffer) > 6)
> - {
> - if ((((CHAR16*)NewBuffer)[(StrSize(NewBuffer)/2) - 2] == CHAR_LINEFEED)
> - && (((CHAR16*)NewBuffer)[(StrSize(NewBuffer)/2) - 3] ==
> CHAR_CARRIAGE_RETURN)) {
> - ((CHAR16*)NewBuffer)[(StrSize(NewBuffer)/2) - 3] = CHAR_NULL;
> + if (TotalSize / sizeof (CHAR16) >= 3) {
> + if ( (((CHAR16*)NewBuffer)[TotalSize / sizeof (CHAR16) - 2] ==
> CHAR_LINEFEED) &&
> + (((CHAR16*)NewBuffer)[TotalSize / sizeof (CHAR16) - 3] ==
> CHAR_CARRIAGE_RETURN)
> + ) {
> + ((CHAR16*)NewBuffer)[TotalSize / sizeof (CHAR16) - 3] =
> + CHAR_NULL;
> }
>
> if (Volatile) {
> - Status =
> SHELL_SET_ENVIRONMENT_VARIABLE_V(((EFI_FILE_PROTOCOL_ENVIRONMEN
> T*)This)->Name, StrSize(NewBuffer), NewBuffer);
> + Status = SHELL_SET_ENVIRONMENT_VARIABLE_V (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + TotalSize - sizeof (CHAR16),
> + NewBuffer
> + );
> +
> + if (!EFI_ERROR(Status)) {
> + Status = ShellAddEnvVarToList (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + NewBuffer,
> + TotalSize,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS
> + );
> + }
> } else {
> - Status =
> SHELL_SET_ENVIRONMENT_VARIABLE_NV(((EFI_FILE_PROTOCOL_ENVIRONME
> NT*)This)->Name, StrSize(NewBuffer), NewBuffer);
> + Status = SHELL_SET_ENVIRONMENT_VARIABLE_NV (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + TotalSize - sizeof (CHAR16),
> + NewBuffer
> + );
> +
> + if (!EFI_ERROR(Status)) {
> + Status = ShellAddEnvVarToList (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + NewBuffer,
> + TotalSize,
> + EFI_VARIABLE_NON_VOLATILE |
> EFI_VARIABLE_BOOTSERVICE_ACCESS
> + );
> + }
> }
> }
> }
> @@ -1128,12 +1157,14 @@ FileInterfaceEnvRead(
>
> /**
> File style interface for Volatile Environment Variable (Write).
> + This function also caches the environment variable into gShellEnvVarList.
>
> @param[in] This The pointer to the EFI_FILE_PROTOCOL object.
> @param[in, out] BufferSize Size in bytes of Buffer.
> @param[in] Buffer The pointer to the buffer to write.
>
> - @retval EFI_SUCCESS The data was read.
> + @retval EFI_SUCCESS The data was successfully write to variable.
> + @retval SHELL_OUT_OF_RESOURCES A memory allocation failed.
> **/
> EFI_STATUS
> EFIAPI
> @@ -1146,41 +1177,71 @@ FileInterfaceEnvVolWrite(
> VOID* NewBuffer;
> UINTN NewSize;
> EFI_STATUS Status;
> + UINTN TotalSize;
>
> NewBuffer = NULL;
> NewSize = 0;
> + TotalSize = 0;
>
> Status =
> SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT*
> )This)->Name, &NewSize, NewBuffer);
> - if (Status == EFI_BUFFER_TOO_SMALL){
> - NewBuffer = AllocateZeroPool(NewSize + *BufferSize + sizeof(CHAR16));
> + if (Status == EFI_BUFFER_TOO_SMALL) {
> + TotalSize = NewSize + *BufferSize + sizeof (CHAR16); } else if
> + (Status == EFI_NOT_FOUND) {
> + TotalSize = *BufferSize + sizeof(CHAR16); } else {
> + return Status;
> + }
> +
> + NewBuffer = AllocateZeroPool (TotalSize); if (NewBuffer == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + if (Status == EFI_BUFFER_TOO_SMALL) {
> Status =
> SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT*
> )This)->Name, &NewSize, NewBuffer);
> }
> - if (!EFI_ERROR(Status) && NewBuffer != NULL) {
> - while (((CHAR16*)NewBuffer)[NewSize/2] == CHAR_NULL) {
> - //
> - // We want to overwrite the CHAR_NULL
> - //
> - NewSize -= 2;
> - }
> - CopyMem((UINT8*)NewBuffer + NewSize + 2, Buffer, *BufferSize);
> - Status =
> SHELL_SET_ENVIRONMENT_VARIABLE_V(((EFI_FILE_PROTOCOL_ENVIRONMEN
> T*)This)->Name, StrSize(NewBuffer), NewBuffer);
> - FreePool(NewBuffer);
> - return (Status);
> - } else {
> - SHELL_FREE_NON_NULL(NewBuffer);
> - return
> (SHELL_SET_ENVIRONMENT_VARIABLE_V(((EFI_FILE_PROTOCOL_ENVIRONME
> NT*)This)->Name, *BufferSize, Buffer));
> +
> + if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND) {
> + FreePool (NewBuffer);
> + return Status;
> + }
> +
> + CopyMem ((UINT8*)NewBuffer + NewSize, Buffer, *BufferSize); Status =
> + ShellAddEnvVarToList (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + NewBuffer,
> + TotalSize,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS
> + );
> + if (EFI_ERROR(Status)) {
> + FreePool (NewBuffer);
> + return Status;
> + }
> +
> + Status = SHELL_SET_ENVIRONMENT_VARIABLE_V (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + TotalSize - sizeof (CHAR16),
> + NewBuffer
> + );
> + if (EFI_ERROR(Status)) {
> + ShellRemvoeEnvVarFromList
> + (((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name);
> }
> +
> + FreePool (NewBuffer);
> + return Status;
> }
>
>
> /**
> File style interface for Non Volatile Environment Variable (Write).
> + This function also caches the environment variable into gShellEnvVarList.
>
> @param[in] This The pointer to the EFI_FILE_PROTOCOL object.
> @param[in, out] BufferSize Size in bytes of Buffer.
> @param[in] Buffer The pointer to the buffer to write.
>
> - @retval EFI_SUCCESS The data was read.
> + @retval EFI_SUCCESS The data was successfully write to variable.
> + @retval SHELL_OUT_OF_RESOURCES A memory allocation failed.
> **/
> EFI_STATUS
> EFIAPI
> @@ -1193,27 +1254,58 @@ FileInterfaceEnvNonVolWrite(
> VOID* NewBuffer;
> UINTN NewSize;
> EFI_STATUS Status;
> + UINTN TotalSize;
>
> NewBuffer = NULL;
> NewSize = 0;
> + TotalSize = 0;
>
> Status =
> SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT*
> )This)->Name, &NewSize, NewBuffer);
> - if (Status == EFI_BUFFER_TOO_SMALL){
> - NewBuffer = AllocateZeroPool(NewSize + *BufferSize);
> + if (Status == EFI_BUFFER_TOO_SMALL) {
> + TotalSize = NewSize + *BufferSize + sizeof (CHAR16); } else if
> + (Status == EFI_NOT_FOUND) {
> + TotalSize = *BufferSize + sizeof (CHAR16); } else {
> + return Status;
> + }
> +
> + NewBuffer = AllocateZeroPool (TotalSize); if (NewBuffer == NULL) {
> + return EFI_OUT_OF_RESOURCES;
> + }
> +
> + if (Status == EFI_BUFFER_TOO_SMALL) {
> Status =
> SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT*
> )This)->Name, &NewSize, NewBuffer);
> }
> - if (!EFI_ERROR(Status)) {
> - CopyMem((UINT8*)NewBuffer + NewSize, Buffer, *BufferSize);
> - return (SHELL_SET_ENVIRONMENT_VARIABLE_NV(
> - ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> - NewSize + *BufferSize,
> - NewBuffer));
> - } else {
> - return (SHELL_SET_ENVIRONMENT_VARIABLE_NV(
> - ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> - *BufferSize,
> - Buffer));
> +
> + if (EFI_ERROR(Status) && Status != EFI_NOT_FOUND) {
> + FreePool (NewBuffer);
> + return Status;
> + }
> +
> + CopyMem ((UINT8*) NewBuffer + NewSize, Buffer, *BufferSize); Status
> + = ShellAddEnvVarToList (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + NewBuffer,
> + TotalSize,
> + EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS
> + );
> + if (EFI_ERROR (Status)) {
> + FreePool (NewBuffer);
> + return Status;
> }
> +
> + Status = SHELL_SET_ENVIRONMENT_VARIABLE_NV (
> + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name,
> + TotalSize - sizeof (CHAR16),
> + NewBuffer
> + );
> + if (EFI_ERROR (Status)) {
> + ShellRemvoeEnvVarFromList
> + (((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name);
> + }
> +
> + FreePool (NewBuffer);
> + return Status;
> }
>
> /**
> diff --git a/ShellPkg/Application/Shell/ShellEnvVar.c
> b/ShellPkg/Application/Shell/ShellEnvVar.c
> index 6d8c79b..4c49c1c 100644
> --- a/ShellPkg/Application/Shell/ShellEnvVar.c
> +++ b/ShellPkg/Application/Shell/ShellEnvVar.c
> @@ -178,7 +178,10 @@ GetEnvironmentVariableList(
> Status = EFI_OUT_OF_RESOURCES;
> } else {
> ValSize = ValBufferSize;
> - VarList->Val = AllocateZeroPool(ValSize);
> + //
> + // We need another CHAR16 to save '\0' in VarList->Val.
> + //
> + VarList->Val = AllocateZeroPool (ValSize + sizeof (CHAR16));
> if (VarList->Val == NULL) {
> SHELL_FREE_NON_NULL(VarList);
> Status = EFI_OUT_OF_RESOURCES; @@ -188,7 +191,10 @@
> GetEnvironmentVariableList(
> if (Status == EFI_BUFFER_TOO_SMALL){
> ValBufferSize = ValSize > ValBufferSize * 2 ? ValSize : ValBufferSize * 2;
> SHELL_FREE_NON_NULL (VarList->Val);
> - VarList->Val = AllocateZeroPool(ValBufferSize);
> + //
> + // We need another CHAR16 to save '\0' in VarList->Val.
> + //
> + VarList->Val = AllocateZeroPool (ValBufferSize + sizeof
> + (CHAR16));
> if (VarList->Val == NULL) {
> SHELL_FREE_NON_NULL(VarList);
> Status = EFI_OUT_OF_RESOURCES; @@ -272,7 +278,7 @@
> SetEnvironmentVariableList(
> ; !IsNull(ListHead, &Node->Link)
> ; Node = (ENV_VAR_LIST*)GetNextNode(ListHead, &Node->Link)
> ){
> - Size = StrSize(Node->Val);
> + Size = StrSize (Node->Val) - sizeof (CHAR16);
> if (Node->Atts & EFI_VARIABLE_NON_VOLATILE) {
> Status = SHELL_SET_ENVIRONMENT_VARIABLE_NV(Node->Key, Size, Node-
> >Val);
> } else {
> diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
> b/ShellPkg/Application/Shell/ShellProtocol.c
> index 04b66c5..12c7c40 100644
> --- a/ShellPkg/Application/Shell/ShellProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellProtocol.c
> @@ -2872,8 +2872,8 @@ InternalEfiShellSetEnv(
> );
> if (!EFI_ERROR (Status)) {
> Status = Volatile
> - ? SHELL_SET_ENVIRONMENT_VARIABLE_V(Name, StrSize(Value), Value)
> - : SHELL_SET_ENVIRONMENT_VARIABLE_NV(Name, StrSize(Value),
> Value);
> + ? SHELL_SET_ENVIRONMENT_VARIABLE_V (Name, StrSize (Value) -
> sizeof (CHAR16), Value)
> + : SHELL_SET_ENVIRONMENT_VARIABLE_NV (Name, StrSize (Value)
> + - sizeof (CHAR16), Value);
> if (EFI_ERROR (Status)) {
> ShellRemvoeEnvVarFromList(Name);
> }
> --
> 2.9.0.windows.1
prev parent reply other threads:[~2016-12-08 21:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-08 1:45 [PATCH] ShellPkg/Application: Fix ">v" cannot update environment variable Ruiyu Ni
2016-12-08 17:03 ` Shah, Tapan
2016-12-08 21:59 ` Carsey, Jaben [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CB6E33457884FA40993F35157061515C54B0EA7B@FMSMSX103.amr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox