From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BBE6A81F52 for ; Thu, 8 Dec 2016 13:59:05 -0800 (PST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 08 Dec 2016 13:59:05 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,321,1477983600"; d="scan'208";a="1079427276" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 08 Dec 2016 13:59:04 -0800 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 8 Dec 2016 13:59:04 -0800 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.12]) by FMSMSX153.amr.corp.intel.com ([169.254.9.101]) with mapi id 14.03.0248.002; Thu, 8 Dec 2016 13:59:04 -0800 From: "Carsey, Jaben" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: "Chen, Chen A" , "Carsey, Jaben" Thread-Topic: [PATCH] ShellPkg/Application: Fix ">v" cannot update environment variable Thread-Index: AQHSUPTHqNlDxUfBIEyLnHz22hS0N6D+mqeg Date: Thu, 8 Dec 2016 21:59:03 +0000 Message-ID: References: <20161208014519.281120-1-ruiyu.ni@intel.com> In-Reply-To: <20161208014519.281120-1-ruiyu.ni@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.1.200.108] MIME-Version: 1.0 Subject: Re: [PATCH] ShellPkg/Application: Fix ">v" cannot update environment variable X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Dec 2016 21:59:05 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jaben Carsey > -----Original Message----- > From: Ni, Ruiyu > Sent: Wednesday, December 7, 2016 5:45 PM > To: edk2-devel@lists.01.org > Cc: Chen, Chen A ; Carsey, Jaben > > Subject: [PATCH] ShellPkg/Application: Fix ">v" cannot update environment > variable > Importance: High >=20 > From: Chen A Chen >=20 > 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 var= iable > storage but forgot to update the cached environment variables in > gShellEnvVarList. > When updating the variable storage, the existing code unnecessary saved t= he > ending NULL character into variable storage. >=20 > The patch fixes all the above issues. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Chen A Chen > Cc: Ruiyu Ni > Cc: Jaben Carsey > --- > 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(-) >=20 > 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; >=20 > // > // Most if not all UEFI commands will have an '\r\n' at the end of any= output. > @@ -1049,6 +1050,7 @@ FileInterfaceEnvClose( > // > NewBuffer =3D NULL; > NewSize =3D 0; > + TotalSize =3D 0; >=20 > Status =3D IsVolatileEnv (((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name= , > &Volatile); > if (EFI_ERROR (Status)) { > @@ -1057,7 +1059,8 @@ FileInterfaceEnvClose( >=20 > Status =3D > SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT* > )This)->Name, &NewSize, NewBuffer); > if (Status =3D=3D EFI_BUFFER_TOO_SMALL) { > - NewBuffer =3D AllocateZeroPool(NewSize + sizeof(CHAR16)); > + TotalSize =3D NewSize + sizeof (CHAR16); > + NewBuffer =3D AllocateZeroPool (TotalSize); > if (NewBuffer =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } > @@ -1066,17 +1069,43 @@ FileInterfaceEnvClose( >=20 > if (!EFI_ERROR(Status) && NewBuffer !=3D NULL) { >=20 > - if (StrSize(NewBuffer) > 6) > - { > - if ((((CHAR16*)NewBuffer)[(StrSize(NewBuffer)/2) - 2] =3D=3D CHAR_= LINEFEED) > - && (((CHAR16*)NewBuffer)[(StrSize(NewBuffer)/2) - 3] =3D=3D > CHAR_CARRIAGE_RETURN)) { > - ((CHAR16*)NewBuffer)[(StrSize(NewBuffer)/2) - 3] =3D CHAR_NULL; > + if (TotalSize / sizeof (CHAR16) >=3D 3) { > + if ( (((CHAR16*)NewBuffer)[TotalSize / sizeof (CHAR16) - 2] =3D=3D > CHAR_LINEFEED) && > + (((CHAR16*)NewBuffer)[TotalSize / sizeof (CHAR16) - 3] =3D=3D > CHAR_CARRIAGE_RETURN) > + ) { > + ((CHAR16*)NewBuffer)[TotalSize / sizeof (CHAR16) - 3] =3D > + CHAR_NULL; > } >=20 > if (Volatile) { > - Status =3D > SHELL_SET_ENVIRONMENT_VARIABLE_V(((EFI_FILE_PROTOCOL_ENVIRONMEN > T*)This)->Name, StrSize(NewBuffer), NewBuffer); > + Status =3D SHELL_SET_ENVIRONMENT_VARIABLE_V ( > + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name, > + TotalSize - sizeof (CHAR16), > + NewBuffer > + ); > + > + if (!EFI_ERROR(Status)) { > + Status =3D ShellAddEnvVarToList ( > + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name, > + NewBuffer, > + TotalSize, > + EFI_VARIABLE_BOOTSERVICE_ACCESS > + ); > + } > } else { > - Status =3D > SHELL_SET_ENVIRONMENT_VARIABLE_NV(((EFI_FILE_PROTOCOL_ENVIRONME > NT*)This)->Name, StrSize(NewBuffer), NewBuffer); > + Status =3D SHELL_SET_ENVIRONMENT_VARIABLE_NV ( > + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name, > + TotalSize - sizeof (CHAR16), > + NewBuffer > + ); > + > + if (!EFI_ERROR(Status)) { > + Status =3D ShellAddEnvVarToList ( > + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name, > + NewBuffer, > + TotalSize, > + EFI_VARIABLE_NON_VOLATILE | > EFI_VARIABLE_BOOTSERVICE_ACCESS > + ); > + } > } > } > } > @@ -1128,12 +1157,14 @@ FileInterfaceEnvRead( >=20 > /** > File style interface for Volatile Environment Variable (Write). > + This function also caches the environment variable into gShellEnvVarLi= st. >=20 > @param[in] This The pointer to the EFI_FILE_PROTOCOL obje= ct. > @param[in, out] BufferSize Size in bytes of Buffer. > @param[in] Buffer The pointer to the buffer to write. >=20 > - @retval EFI_SUCCESS The data was read. > + @retval EFI_SUCCESS The data was successfully write to var= iable. > + @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; >=20 > NewBuffer =3D NULL; > NewSize =3D 0; > + TotalSize =3D 0; >=20 > Status =3D > SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT* > )This)->Name, &NewSize, NewBuffer); > - if (Status =3D=3D EFI_BUFFER_TOO_SMALL){ > - NewBuffer =3D AllocateZeroPool(NewSize + *BufferSize + sizeof(CHAR16= )); > + if (Status =3D=3D EFI_BUFFER_TOO_SMALL) { > + TotalSize =3D NewSize + *BufferSize + sizeof (CHAR16); } else if > + (Status =3D=3D EFI_NOT_FOUND) { > + TotalSize =3D *BufferSize + sizeof(CHAR16); } else { > + return Status; > + } > + > + NewBuffer =3D AllocateZeroPool (TotalSize); if (NewBuffer =3D=3D NULL= ) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + if (Status =3D=3D EFI_BUFFER_TOO_SMALL) { > Status =3D > SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT* > )This)->Name, &NewSize, NewBuffer); > } > - if (!EFI_ERROR(Status) && NewBuffer !=3D NULL) { > - while (((CHAR16*)NewBuffer)[NewSize/2] =3D=3D CHAR_NULL) { > - // > - // We want to overwrite the CHAR_NULL > - // > - NewSize -=3D 2; > - } > - CopyMem((UINT8*)NewBuffer + NewSize + 2, Buffer, *BufferSize); > - Status =3D > 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 !=3D EFI_NOT_FOUND) { > + FreePool (NewBuffer); > + return Status; > + } > + > + CopyMem ((UINT8*)NewBuffer + NewSize, Buffer, *BufferSize); Status = =3D > + ShellAddEnvVarToList ( > + ((EFI_FILE_PROTOCOL_ENVIRONMENT*)This)->Name, > + NewBuffer, > + TotalSize, > + EFI_VARIABLE_BOOTSERVICE_ACCESS > + ); > + if (EFI_ERROR(Status)) { > + FreePool (NewBuffer); > + return Status; > + } > + > + Status =3D 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; > } >=20 >=20 > /** > File style interface for Non Volatile Environment Variable (Write). > + This function also caches the environment variable into gShellEnvVarLi= st. >=20 > @param[in] This The pointer to the EFI_FILE_PROTOCOL obje= ct. > @param[in, out] BufferSize Size in bytes of Buffer. > @param[in] Buffer The pointer to the buffer to write. >=20 > - @retval EFI_SUCCESS The data was read. > + @retval EFI_SUCCESS The data was successfully write to var= iable. > + @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; >=20 > NewBuffer =3D NULL; > NewSize =3D 0; > + TotalSize =3D 0; >=20 > Status =3D > SHELL_GET_ENVIRONMENT_VARIABLE(((EFI_FILE_PROTOCOL_ENVIRONMENT* > )This)->Name, &NewSize, NewBuffer); > - if (Status =3D=3D EFI_BUFFER_TOO_SMALL){ > - NewBuffer =3D AllocateZeroPool(NewSize + *BufferSize); > + if (Status =3D=3D EFI_BUFFER_TOO_SMALL) { > + TotalSize =3D NewSize + *BufferSize + sizeof (CHAR16); } else if > + (Status =3D=3D EFI_NOT_FOUND) { > + TotalSize =3D *BufferSize + sizeof (CHAR16); } else { > + return Status; > + } > + > + NewBuffer =3D AllocateZeroPool (TotalSize); if (NewBuffer =3D=3D NULL= ) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + if (Status =3D=3D EFI_BUFFER_TOO_SMALL) { > Status =3D > 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 !=3D EFI_NOT_FOUND) { > + FreePool (NewBuffer); > + return Status; > + } > + > + CopyMem ((UINT8*) NewBuffer + NewSize, Buffer, *BufferSize); Status > + =3D 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 =3D 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; > } >=20 > /** > 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 =3D EFI_OUT_OF_RESOURCES; > } else { > ValSize =3D ValBufferSize; > - VarList->Val =3D AllocateZeroPool(ValSize); > + // > + // We need another CHAR16 to save '\0' in VarList->Val. > + // > + VarList->Val =3D AllocateZeroPool (ValSize + sizeof (CHAR16)); > if (VarList->Val =3D=3D NULL) { > SHELL_FREE_NON_NULL(VarList); > Status =3D EFI_OUT_OF_RESOURCES; @@ -188,7 +191,10 @@ > GetEnvironmentVariableList( > if (Status =3D=3D EFI_BUFFER_TOO_SMALL){ > ValBufferSize =3D ValSize > ValBufferSize * 2 ? ValSize : ValB= ufferSize * 2; > SHELL_FREE_NON_NULL (VarList->Val); > - VarList->Val =3D AllocateZeroPool(ValBufferSize); > + // > + // We need another CHAR16 to save '\0' in VarList->Val. > + // > + VarList->Val =3D AllocateZeroPool (ValBufferSize + sizeof > + (CHAR16)); > if (VarList->Val =3D=3D NULL) { > SHELL_FREE_NON_NULL(VarList); > Status =3D EFI_OUT_OF_RESOURCES; @@ -272,7 +278,7 @@ > SetEnvironmentVariableList( > ; !IsNull(ListHead, &Node->Link) > ; Node =3D (ENV_VAR_LIST*)GetNextNode(ListHead, &Node->Link) > ){ > - Size =3D StrSize(Node->Val); > + Size =3D StrSize (Node->Val) - sizeof (CHAR16); > if (Node->Atts & EFI_VARIABLE_NON_VOLATILE) { > Status =3D 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 =3D Volatile > - ? SHELL_SET_ENVIRONMENT_VARIABLE_V(Name, StrSize(Value), Va= lue) > - : 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