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

[Gabriel Ivăncescu]

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.