Re: [edk2-devel] [PATCH] ShellPkg/UefiShellLevel2CommansLib: Pointer Resonse should be checked
Reviewed-by: Jaben Carsey Thanks -Jaben > -Original Message- > From: Gao, Zhichao > Sent: Sunday, August 11, 2019 5:36 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray > Subject: [PATCH] ShellPkg/UefiShellLevel2CommansLib: Pointer Resonse > should be checked > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2049 > > ShellPkg\Library\UefiShellLevel2CommandsLib\Cp.c line 104 and > ShellPkg\Library\UefiShellLevel2CommandsLib\Mv.c line 640, the > pointer variable Response may be a NULL pointer. So we should > make sure that it isn't NULL before dereference it. > > If Response is NULL that indicates a EFI_OUT_OF_RESOURCES > error, directly return SHELL_ABORTED. > > Cc: Jaben Carsey > Cc: Ray Ni > Signed-off-by: Zhichao Gao > --- > ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c | 5 - > ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c | 5 - > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > index 18b05b5803..4a2c2cfe64 100644 > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > @@ -2,7 +2,7 @@ >Main file for cp shell level 2 function. > >(C) Copyright 2015 Hewlett-Packard Development Company, L.P. > - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. > + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. >SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -101,6 +101,9 @@ CopySingleFile( > // possibly return based on response > // > if (!SilentMode) { > + if (Response == NULL) { > +return SHELL_ABORTED; > + } >switch (*(SHELL_PROMPT_RESPONSE*)Response) { > case ShellPromptResponseNo: >// > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > index 8c2852d7eb..f50c1e4c20 100644 > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > @@ -2,7 +2,7 @@ >Main file for mv shell level 2 function. > >(C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P. > - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. > + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. >SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -637,6 +637,9 @@ ValidateAndMoveFiles( >if (Response == NULL) { > ShellPromptForResponseHii(ShellPromptResponseTypeYesNoAllCancel, > STRING_TOKEN (STR_GEN_DEST_EXIST_OVR), gShellLevel2HiiHandle, > ); >} > + if (Response == NULL) { > +return SHELL_ABORTED; > + } >switch (*(SHELL_PROMPT_RESPONSE*)Response) { > case ShellPromptResponseNo: >FreePool(Response); > -- > 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45453): https://edk2.groups.io/g/devel/message/45453 Mute This Topic: https://groups.io/mt/32835761/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH] ShellPkg/UefiShellLevel2CommansLib: Pointer Resonse should be checked
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2049 ShellPkg\Library\UefiShellLevel2CommandsLib\Cp.c line 104 and ShellPkg\Library\UefiShellLevel2CommandsLib\Mv.c line 640, the pointer variable Response may be a NULL pointer. So we should make sure that it isn't NULL before dereference it. If Response is NULL that indicates a EFI_OUT_OF_RESOURCES error, directly return SHELL_ABORTED. Cc: Jaben Carsey Cc: Ray Ni Signed-off-by: Zhichao Gao --- ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c | 5 - ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c | 5 - 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c index 18b05b5803..4a2c2cfe64 100644 --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c @@ -2,7 +2,7 @@ Main file for cp shell level 2 function. (C) Copyright 2015 Hewlett-Packard Development Company, L.P. - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -101,6 +101,9 @@ CopySingleFile( // possibly return based on response // if (!SilentMode) { + if (Response == NULL) { +return SHELL_ABORTED; + } switch (*(SHELL_PROMPT_RESPONSE*)Response) { case ShellPromptResponseNo: // diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c index 8c2852d7eb..f50c1e4c20 100644 --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c @@ -2,7 +2,7 @@ Main file for mv shell level 2 function. (C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P. - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -637,6 +637,9 @@ ValidateAndMoveFiles( if (Response == NULL) { ShellPromptForResponseHii(ShellPromptResponseTypeYesNoAllCancel, STRING_TOKEN (STR_GEN_DEST_EXIST_OVR), gShellLevel2HiiHandle, ); } + if (Response == NULL) { +return SHELL_ABORTED; + } switch (*(SHELL_PROMPT_RESPONSE*)Response) { case ShellPromptResponseNo: FreePool(Response); -- 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45326): https://edk2.groups.io/g/devel/message/45326 Mute This Topic: https://groups.io/mt/32835761/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] ShellPkg/UefiShellLevel2CommansLib: Pointer Resonse should be checked
That looks good, but I think it would be easier to just do the logic opposite. Cant we just add the ~"if it is NULL, then return" and not have to change anything else? No real need for else in that case since there is a return statement... Hand written pseudo-patch follows: if (!SilentMode) { +if (Response == NULL) { +return SHELL_ABORTED; +} switch (*(SHELL_PROMPT_RESPONSE*)Response) { Thanks -Jaben > -Original Message- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Gao, Zhichao > Sent: Wednesday, August 07, 2019 5:54 PM > To: devel@edk2.groups.io > Cc: Carsey, Jaben ; Ni, Ray > Subject: [edk2-devel] [PATCH] ShellPkg/UefiShellLevel2CommansLib: Pointer > Resonse should be checked > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2049 > > ShellPkg\Library\UefiShellLevel2CommandsLib\Cp.c line 104 and > ShellPkg\Library\UefiShellLevel2CommandsLib\Mv.c line 640, the > pointer variable Response may be a NULL pointer. So we should > make sure that it isn't NULL before dereference it. > > If Response is NULL that indicates a EFI_OUT_OF_RESOURCES > error, directly return SHELL_ABORTED. > > Cc: Jaben Carsey > Cc: Ray Ni > Signed-off-by: Zhichao Gao > --- > .../Library/UefiShellLevel2CommandsLib/Cp.c | 42 --- > .../Library/UefiShellLevel2CommandsLib/Mv.c | 52 ++- > 2 files changed, 51 insertions(+), 43 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > index 18b05b5803..632d50229a 100644 > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c > @@ -2,7 +2,7 @@ >Main file for cp shell level 2 function. > >(C) Copyright 2015 Hewlett-Packard Development Company, L.P. > - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. > + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. >SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -101,24 +101,28 @@ CopySingleFile( > // possibly return based on response > // > if (!SilentMode) { > - switch (*(SHELL_PROMPT_RESPONSE*)Response) { > -case ShellPromptResponseNo: > - // > - // return success here so we dont stop the process > - // > - return (SHELL_SUCCESS); > -case ShellPromptResponseCancel: > - *Resp = Response; > - // > - // indicate to stop everything > - // > - return (SHELL_ABORTED); > -case ShellPromptResponseAll: > - *Resp = Response; > -case ShellPromptResponseYes: > - break; > -default: > - return SHELL_ABORTED; > + if (Response != NULL) { > +switch (*(SHELL_PROMPT_RESPONSE*)Response) { > + case ShellPromptResponseNo: > +// > +// return success here so we dont stop the process > +// > +return (SHELL_SUCCESS); > + case ShellPromptResponseCancel: > +*Resp = Response; > +// > +// indicate to stop everything > +// > +return (SHELL_ABORTED); > + case ShellPromptResponseAll: > +*Resp = Response; > + case ShellPromptResponseYes: > +break; > + default: > +return SHELL_ABORTED; > +} > + } else { > +return SHELL_ABORTED; >} > } >} > diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > index 8c2852d7eb..2cfa588a8c 100644 > --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c > @@ -2,7 +2,7 @@ >Main file for mv shell level 2 function. > >(C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P. > - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. > + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. >SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -637,29 +637,33 @@ ValidateAndMoveFiles( >if (Response == NULL) { > ShellPromptForResponseHii(ShellPromptResponseTypeYesNoAllCancel, > STRING_TOKEN (STR_GEN_DEST_EXIST_OVR), gShellLevel2HiiHandle, > ); >} > - switch (*(SHELL_PROMPT_RESPONSE*)Response) { > -case ShellPromptResponseNo: > - FreePool(Response); > - Response = NULL; > - continue; > -case ShellPromptResponseCancel: > - *Resp = Res
[edk2-devel] [PATCH] ShellPkg/UefiShellLevel2CommansLib: Pointer Resonse should be checked
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2049 ShellPkg\Library\UefiShellLevel2CommandsLib\Cp.c line 104 and ShellPkg\Library\UefiShellLevel2CommandsLib\Mv.c line 640, the pointer variable Response may be a NULL pointer. So we should make sure that it isn't NULL before dereference it. If Response is NULL that indicates a EFI_OUT_OF_RESOURCES error, directly return SHELL_ABORTED. Cc: Jaben Carsey Cc: Ray Ni Signed-off-by: Zhichao Gao --- .../Library/UefiShellLevel2CommandsLib/Cp.c | 42 --- .../Library/UefiShellLevel2CommandsLib/Mv.c | 52 ++- 2 files changed, 51 insertions(+), 43 deletions(-) diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c index 18b05b5803..632d50229a 100644 --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Cp.c @@ -2,7 +2,7 @@ Main file for cp shell level 2 function. (C) Copyright 2015 Hewlett-Packard Development Company, L.P. - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -101,24 +101,28 @@ CopySingleFile( // possibly return based on response // if (!SilentMode) { - switch (*(SHELL_PROMPT_RESPONSE*)Response) { -case ShellPromptResponseNo: - // - // return success here so we dont stop the process - // - return (SHELL_SUCCESS); -case ShellPromptResponseCancel: - *Resp = Response; - // - // indicate to stop everything - // - return (SHELL_ABORTED); -case ShellPromptResponseAll: - *Resp = Response; -case ShellPromptResponseYes: - break; -default: - return SHELL_ABORTED; + if (Response != NULL) { +switch (*(SHELL_PROMPT_RESPONSE*)Response) { + case ShellPromptResponseNo: +// +// return success here so we dont stop the process +// +return (SHELL_SUCCESS); + case ShellPromptResponseCancel: +*Resp = Response; +// +// indicate to stop everything +// +return (SHELL_ABORTED); + case ShellPromptResponseAll: +*Resp = Response; + case ShellPromptResponseYes: +break; + default: +return SHELL_ABORTED; +} + } else { +return SHELL_ABORTED; } } } diff --git a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c index 8c2852d7eb..2cfa588a8c 100644 --- a/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c +++ b/ShellPkg/Library/UefiShellLevel2CommandsLib/Mv.c @@ -2,7 +2,7 @@ Main file for mv shell level 2 function. (C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P. - Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved. + Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved. SPDX-License-Identifier: BSD-2-Clause-Patent **/ @@ -637,29 +637,33 @@ ValidateAndMoveFiles( if (Response == NULL) { ShellPromptForResponseHii(ShellPromptResponseTypeYesNoAllCancel, STRING_TOKEN (STR_GEN_DEST_EXIST_OVR), gShellLevel2HiiHandle, ); } - switch (*(SHELL_PROMPT_RESPONSE*)Response) { -case ShellPromptResponseNo: - FreePool(Response); - Response = NULL; - continue; -case ShellPromptResponseCancel: - *Resp = Response; - // - // indicate to stop everything - // - SHELL_FREE_NON_NULL(FullCwd); - return (SHELL_ABORTED); -case ShellPromptResponseAll: - *Resp = Response; - break; -case ShellPromptResponseYes: - FreePool(Response); - Response = NULL; - break; -default: - FreePool(Response); - SHELL_FREE_NON_NULL(FullCwd); - return SHELL_ABORTED; + if (Response != NULL) { +switch (*(SHELL_PROMPT_RESPONSE*)Response) { + case ShellPromptResponseNo: +FreePool(Response); +Response = NULL; +continue; + case ShellPromptResponseCancel: +*Resp = Response; +// +// indicate to stop everything +// +SHELL_FREE_NON_NULL(FullCwd); +return (SHELL_ABORTED); + case ShellPromptResponseAll: +*Resp = Response; +break; + case ShellPromptResponseYes: +FreePool(Response); +Response = NULL; +break; + default: +FreePool(Response); +SHELL_FREE_NON_NULL(FullCwd); +return SHELL_ABORTED; +} + } else { +return SHELL_ABORTED; } Status