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

[Gabriel Ivăncescu]

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.