On 17 September 2015 at 14:27, Qiu Shumin <shumin....@intel.com> wrote:
> Difference with previous version:
> It's not correct to casts away constness to allow TrimSpaces() to modify
> 'commandline'.
> This version make a copy of 'commandLine' and work with that in the remainder
> of the function.
>
OK, much better. Still some comments below, though.
> Cc: Jaben Carsey <jaben.car...@intel.com>
> Cc: Ruiyu Ni <ruiyu...@intel.com>
> Cc: Yang Jadis <jadis.y...@intel.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qiu Shumin <shumin....@intel.com>
> ---
> ShellPkg/Application/Shell/ShellParametersProtocol.c | 20
> ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/ShellPkg/Application/Shell/ShellParametersProtocol.c
> b/ShellPkg/Application/Shell/ShellParametersProtocol.c
> index b404987..b96405d 100644
> --- a/ShellPkg/Application/Shell/ShellParametersProtocol.c
> +++ b/ShellPkg/Application/Shell/ShellParametersProtocol.c
> @@ -195,6 +195,7 @@ ParseCommandLineToArgs(
> CHAR16 *TempParameter;
> CHAR16 *Walker;
> CHAR16 *NewParam;
> + CHAR16 *NewCommandLine;
> UINTN Size;
>
> ASSERT(Argc != NULL);
> @@ -206,15 +207,22 @@ ParseCommandLineToArgs(
> return (EFI_SUCCESS);
> }
>
> - TrimSpaces(&(CHAR16*)CommandLine);
> - Size = StrSize(CommandLine);
> + NewCommandLine = AllocateCopyPool(StrSize(CommandLine), CommandLine);
> + if (NewCommandLine == NULL){
> + SHELL_FREE_NON_NULL(NewCommandLine);
What is the point of this?
> + return (EFI_OUT_OF_RESOURCES);
> + }
> +
> + TrimSpaces(&NewCommandLine);
> + Size = StrSize(NewCommandLine);
> TempParameter = AllocateZeroPool(Size);
> if (TempParameter == NULL) {
> + SHELL_FREE_NON_NULL(NewCommandLine);
> return (EFI_OUT_OF_RESOURCES);
> }
>
> for ( Count = 0
> - , Walker = (CHAR16*)CommandLine
> + , Walker = (CHAR16*)NewCommandLine
> ; Walker != NULL && *Walker != CHAR_NULL
> ; Count++
> ) {
> @@ -229,21 +237,24 @@ ParseCommandLineToArgs(
> (*Argv) = AllocateZeroPool((Count)*sizeof(CHAR16*));
> if (*Argv == NULL) {
> SHELL_FREE_NON_NULL(TempParameter);
> + SHELL_FREE_NON_NULL(NewCommandLine);
> return (EFI_OUT_OF_RESOURCES);
> }
>
> *Argc = 0;
> - Walker = (CHAR16*)CommandLine;
> + Walker = (CHAR16*)NewCommandLine;
> while(Walker != NULL && *Walker != CHAR_NULL) {
> SetMem16(TempParameter, Size, CHAR_NULL);
> if (EFI_ERROR(GetNextParameter(&Walker, &TempParameter, Size))) {
> SHELL_FREE_NON_NULL(TempParameter);
> + SHELL_FREE_NON_NULL(NewCommandLine);
> return (EFI_INVALID_PARAMETER);
> }
>
> NewParam = AllocateCopyPool(StrSize(TempParameter), TempParameter);
> if (NewParam == NULL){
> SHELL_FREE_NON_NULL(TempParameter);
> + SHELL_FREE_NON_NULL(NewCommandLine);
> return (EFI_OUT_OF_RESOURCES);
> }
> ((CHAR16**)(*Argv))[(*Argc)] = NewParam;
> @@ -251,6 +262,7 @@ ParseCommandLineToArgs(
> }
> ASSERT(Count >= (*Argc));
> SHELL_FREE_NON_NULL(TempParameter);
> + SHELL_FREE_NON_NULL(NewCommandLine);
You are now freeing two allocations in 4 places. Wouldn't it be better
to restructure this?
This is a nice to have, though, since this patch fixes a broken build
for all GCC users.
So even without the restructuring, but *with* the other change above:
Reviewed-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
Thanks,
Ard.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
