Re: avifil32: Simplify comparing two unsigned ints (PVS-Studio) (try 2)

[Ken Thomases]

On Nov 10, 2014, at 4:56 PM, André Hentschel  wrote:
> try 2: added the missing +1 thanks to a hint by ken on irc
>
> dlls/avifil32/avifile.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/avifil32/avifile.c b/dlls/avifil32/avifile.c
> index 6295c7b..3d90e20 100644
> --- a/dlls/avifil32/avifile.c
> +++ b/dlls/avifil32/avifile.c
> @@ -466,9 +466,9 @@ static HRESULT WINAPI IAVIFile_fnDeleteStream(IAVIFile *iface, DWORD fccType, LO
>      /* ... so delete it now */
>      HeapFree(GetProcessHeap(), 0, This->ppStreams[nStream]);
> - if (This->fInfo.dwStreams - nStream > 0)
> -
>  memcpy(This->ppStreams + nStream, This->ppStreams + nStream + 1,
> -
>     (This->fInfo.dwStreams - nStream) * sizeof(IAVIStreamImpl*));
> -
>
> if (nStream < This->fInfo.dwStreams)
> -
>  memcpy(&This->ppStreams[nStream], &This->ppStreams[nStream + 1],
> -
>         (This->fInfo.dwStreams - nStream) * sizeof(This->ppStreams[0]));
>
> This->ppStreams[This->fInfo.dwStreams] = NULL;
>    This->fInfo.dwStreams--;

This is a more faithful refactoring of the original logic.  However, looking at it some more, it looks like the original logic was wrong.  It's removing an entry from the array and shifting the following entries up to fill the gap.  There are This->fInfo.dwStreams - nStream entries from the nStream'th one to the end.  There are only This->fInfo.dwStreams - (nStream + 1) entries that actually _follow_ the nStream'th one.  The original logic read past the end of the array and this logic still does.

-Ken

avifil32: Simplify comparing two unsigned ints (PVS-Studio) (try 2)

[Ken Thomases]

On Nov 10, 2014, at 5:09 PM, Ken Thomases  wrote:
> On Nov 10, 2014, at 4:56 PM, André Hentschel  wrote:
> > try 2: added the missing +1 thanks to a hint by ken on irc
> >
> > dlls/avifil32/avifile.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/dlls/avifil32/avifile.c b/dlls/avifil32/avifile.c
> > index 6295c7b..3d90e20 100644
> > --- a/dlls/avifil32/avifile.c
> > +++ b/dlls/avifil32/avifile.c
> > @@ -466,9 +466,9 @@ static HRESULT WINAPI IAVIFile_fnDeleteStream(IAVIFile *iface, DWORD fccType, LO
> >     /* ... so delete it now */
> >     HeapFree(GetProcessHeap(), 0, This->ppStreams[nStream]);
> > - if (This->fInfo.dwStreams - nStream > 0)
> > -
> >  memcpy(This->ppStreams + nStream, This->ppStreams + nStream + 1,
> > -
> >     (This->fInfo.dwStreams - nStream) * sizeof(IAVIStreamImpl*));
> > -
> >
> > if (nStream < This->fInfo.dwStreams)
> > -
> >  memcpy(&This->ppStreams[nStream], &This->ppStreams[nStream + 1],
> > -
> >         (This->fInfo.dwStreams - nStream) * sizeof(This->ppStreams[0]));
> >
> > This->ppStreams[This->fInfo.dwStreams] = NULL;
> >   This->fInfo.dwStreams--;
>
> This is a more faithful refactoring of the original logic.  However, looking at it some more, it looks like the original logic was wrong.  It's removing an entry from the array and shifting the following entries up to fill the gap.  There are This->fInfo.dwStreams - nStream entries from the nStream'th one to the end.  There are only This->fInfo.dwStreams - (nStream + 1) entries that actually _follow_ the nStream'th one.  The original logic read past the end of the array and this logic still does.

And Matteo pointed out on IRC that the code should use memmove() for overlapping ranges.

-Ken