[PATCH 1/5] comctl32/listbox: Check for out of bounds index using an unsigned index

[Huw Davies]

On Thu, Feb 07, 2019 at 01:57:09PM +0200, Gabriel Ivăncescu wrote:
> On 2/7/19 1:54 PM, Gabriel Ivăncescu wrote:
> > On 2/7/19 1:29 PM, Huw Davies wrote:
> > > On Thu, Feb 07, 2019 at 01:13:36PM +0200, Gabriel Ivăncescu wrote:
> > > > On 2/7/19 11:52 AM, Huw Davies wrote:
> > > > > On Thu, Jan 31, 2019 at 05:23:19PM +0200, Gabriel Ivăncescu wrote:
> > > > > > Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> > > > > > ---
> > > > > >    dlls/comctl32/listbox.c | 4 ++--
> > > > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/dlls/comctl32/listbox.c b/dlls/comctl32/listbox.c
> > > > > > index cb645b4..71f1c05 100644
> > > > > > --- a/dlls/comctl32/listbox.c
> > > > > > +++ b/dlls/comctl32/listbox.c
> > > > > > @@ -489,7 +489,7 @@ static INT LISTBOX_GetItemFromPoint(
> > > > > > const LB_DESCR *descr, INT x, INT y )
> > > > > >     * Paint an item.
> > > > > >     */
> > > > > >    static void LISTBOX_PaintItem( LB_DESCR *descr, HDC
> > > > > > hdc, const RECT *rect,
> > > > > > -                   INT index, UINT action, BOOL ignoreFocus )
> > > > > > +                   UINT index, UINT action, BOOL ignoreFocus )
> > > > > 
> > > > > Could you leave this change out?  descr->nb_items itself is
> > > > > signed as is the index
> > > > > parameter of LISTBOX_RePaintItem().
> > > > > 
> > > > 
> > > > Hi Huw,
> > > > 
> > > > We have to make it unsigned because checking for bounds then only really
> > > > needs one comparison, as Nikolay suggested awhile ago. And the
> > > > bounds check
> > > > will be needed for LBS_NODATA later (can't test for NULL as that
> > > > would be
> > > > wrong when the item is 0 and nodata).
> > > 
> > > How does LISTBOX_PaintItem() get called with index < 0 ?  If it did, we'd
> > > already be in trouble.
> > > 
> > > My point is really that if we're going to fix the signess of index,
> > > we need to
> > > go through and fix it properly in all the code, not just one
> > > parameter at a
> > > time.  And then it should be a separate patch.
> > > 
> > > Huw.
> > > 
> > 
> > I think the index *can* be negative in many cases, so not sure if it can
> > be easily changed in other parts of the code (or at least it can be -1).
> > 
> > In this particular case, since we only check for bounds, it made more
> > sense to treat it as unsigned (less comparisons and clutter in the
> > code), or at least it's what Nikolay initially suggested if I recall
> > correctly.
> > 
> > Now my question is: if I keep it signed, should I change the check to
> > something like:
> > 
> >    if (index < 0 || index >= descr->nb_items)
> > 
> > or keep it as just  if (index >= descr->nb_items)  like now, and assume
> > the index won't be negative? It's an ERR path if it happens, so I
> > assumed it has a reason to be there.
> 
> Sorry I meant: the index can be negative in many parts of the code. *Not*
> that it can be negative for PaintItem -- I don't know about the latter. Was
> just playing it safe.

The current code assumes index >= 0 (otherwise item will be invalid)
so, for this patch, it's ok to carry on that assumption.

Huw.