Re: [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read

["Wang, Jian J" <jian.j.wang@intel.com>]

Fair enough. It'll be updated in v2.

Regards,
Jian


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, January 24, 2018 11:36 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
> 
> On 1/23/2018 10:14 AM, Jian J Wang wrote:
> > In the for-loop condition of original code, the expression
> >
> >    *CurrentCommand != CHAR_NULL
> >
> > is put before expression
> >
> >    CurrentCommand < SortedCommandList +
> SortedCommandListSize/sizeof(CHAR16)
> >
> > When CurrentCommand walks to the end of string buffer, one more character
> > over the end of string buffer will be read and then stop.
> >
> > To fix this issue, just move the last expression to the first one. Because
> > of short-circuit evaludation of and-expression, the following one
> >
> >    *CurrentCommand != CHAR_NULL
> >
> > will not be evaluated if the expression before it is evaludated as FALSE.
> >
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >   ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> > index a71ade3a20..75e3d74200 100644
> > --- a/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> > +++ b/ShellPkg/Library/UefiShellLevel3CommandsLib/Help.c
> > @@ -397,7 +397,7 @@ ShellCommandRunHelp (
> >           CopyListOfCommandNamesWithDynamic(&SortedCommandList,
> &SortedCommandListSize);
> >
> >           for (CurrentCommand = SortedCommandList
> > -          ; CurrentCommand != NULL && *CurrentCommand != CHAR_NULL &&
> CurrentCommand < SortedCommandList +
> SortedCommandListSize/sizeof(CHAR16)
> > +          ; CurrentCommand < SortedCommandList +
> SortedCommandListSize/sizeof(CHAR16) && CurrentCommand != NULL &&
> *CurrentCommand != CHAR_NULL
> >             ; CurrentCommand += StrLen(CurrentCommand) + 1
> >             ) {
> >             //
> >
> How about keep "CurrentCommand != NULL" in the beginning? I agree to
> swap the other two checks.
> 
> --
> Thanks,
> Ray