Re: wordpad: Avoid buffer overrun in registry_set_filelist.

[Huw Davies]

On Sun, Jun 26, 2016 at 11:42:31AM +0200, Thomas Faber wrote:
> From e80a75cfe223a82eea4c55874975e54de4981282 Mon Sep 17 00:00:00 2001
> From: Thomas Faber <thomas.faber(a)reactos.org>
> Date: Sun, 26 Jun 2016 10:59:08 +0200
> Subject: wordpad: Avoid buffer overrun in registry_set_filelist.
>
> Signed-off-by: Thomas Faber <thomas.faber(a)reactos.org>
> ---
>  programs/wordpad/registry.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/programs/wordpad/registry.c b/programs/wordpad/registry.c
> index bf2af5b..e047f37 100644
> --- a/programs/wordpad/registry.c
> +++ b/programs/wordpad/registry.c
> @@ -298,7 +298,7 @@ void registry_set_filelist(LPCWSTR newFile, HWND hMainWnd)
>                  if(!lstrcmpiW(pFiles[i], newFile))
>                  {
>                      int j;
> -                    for(j = 0; pFiles[j] && j < i; j++)
> +                    for(j = 0; j < i && pFiles[j]; j++)
>                      {
>                          pFiles[i-j] = pFiles[i-j-1];
>                      }

This looks odd to me.  I don't see how this could avoid an overflow;
pFiles[i] is valid at this point and so pFiles[j] will be ok if j == i.

More confusingly though, is why pFiles[j] is in the test at all.

Huw.

wordpad: Avoid buffer overrun in registry_set_filelist.

[Thomas Faber]

On 2016-06-27 09:53, Huw Davies wrote:
> On Sun, Jun 26, 2016 at 11:42:31AM +0200, Thomas Faber wrote:
> > @@ -298,7 +298,7 @@ void registry_set_filelist(LPCWSTR newFile, HWND hMainWnd)
> >                  if(!lstrcmpiW(pFiles[i], newFile))
> >                  {
> >                      int j;
> > -                    for(j = 0; pFiles[j] && j < i; j++)
> > +                    for(j = 0; j < i && pFiles[j]; j++)
> >                      {
> >                          pFiles[i-j] = pFiles[i-j-1];
> >                      }
>
> This looks odd to me.  I don't see how this could avoid an overflow;
> pFiles[i] is valid at this point and so pFiles[j] will be ok if j == i.
>
> More confusingly though, is why pFiles[j] is in the test at all.

Oops, you're completely right, thanks for catching that.

I agree that pFiles[j] cannot be NULL here. I'll send a patch to remove
the check.

Thanks.
-Thomas