* [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
@ 2018-01-23 2:14 Jian J Wang
2018-01-24 3:35 ` Ni, Ruiyu
0 siblings, 1 reply; 5+ messages in thread
From: Jian J Wang @ 2018-01-23 2:14 UTC (permalink / raw)
To: edk2-devel; +Cc: Ruiyu Ni
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
) {
//
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
2018-01-23 2:14 Jian J Wang
@ 2018-01-24 3:35 ` Ni, Ruiyu
2018-01-24 3:40 ` Wang, Jian J
0 siblings, 1 reply; 5+ messages in thread
From: Ni, Ruiyu @ 2018-01-24 3:35 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
2018-01-24 3:35 ` Ni, Ruiyu
@ 2018-01-24 3:40 ` Wang, Jian J
0 siblings, 0 replies; 5+ messages in thread
From: Wang, Jian J @ 2018-01-24 3:40 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
@ 2018-01-24 4:50 Jian J Wang
2018-01-24 5:47 ` Ni, Ruiyu
0 siblings, 1 reply; 5+ messages in thread
From: Jian J Wang @ 2018-01-24 4:50 UTC (permalink / raw)
To: edk2-devel; +Cc: Ruiyu Ni
> v2:
> Keep condition "CurrentCommand != NULL" as the first one.
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..f6159c1335 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 != NULL && CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16) && *CurrentCommand != CHAR_NULL
; CurrentCommand += StrLen(CurrentCommand) + 1
) {
//
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read
2018-01-24 4:50 [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read Jian J Wang
@ 2018-01-24 5:47 ` Ni, Ruiyu
0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ruiyu @ 2018-01-24 5:47 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
On 1/24/2018 12:50 PM, Jian J Wang wrote:
>> v2:
>> Keep condition "CurrentCommand != NULL" as the first one.
>
> 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..f6159c1335 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 != NULL && CurrentCommand < SortedCommandList + SortedCommandListSize/sizeof(CHAR16) && *CurrentCommand != CHAR_NULL
> ; CurrentCommand += StrLen(CurrentCommand) + 1
> ) {
> //
>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-24 5:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-24 4:50 [PATCH] ShellPkg/UefiShellLevel3CommandsLib: fix string over-read Jian J Wang
2018-01-24 5:47 ` Ni, Ruiyu
-- strict thread matches above, loose matches on Subject: below --
2018-01-23 2:14 Jian J Wang
2018-01-24 3:35 ` Ni, Ruiyu
2018-01-24 3:40 ` Wang, Jian J
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox