Re: [PATCH v2 1/2] util/main-loop: Fix maximum number of wait objects for win32

[Bin Meng]

On Wed, Aug 10, 2022 at 11:57 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
>
>
> On Wed, Aug 10, 2022 at 7:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Wed, Aug 10, 2022 at 1:06 AM Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Tue, Aug 9, 2022 at 8:43 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> >>
>> >> From: Bin Meng <bin.meng@windriver.com>
>> >>
>> >> The maximum number of wait objects for win32 should be
>> >> MAXIMUM_WAIT_OBJECTS, not MAXIMUM_WAIT_OBJECTS + 1.
>> >>
>> >> Fix the logic in qemu_add_wait_object() to avoid adding
>> >> the same HANDLE twice.
>> >>
>> >
>> > Please make that a separate patch.
>> >
>> >>
>> >> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>> >> ---
>> >>
>> >> Changes in v2:
>> >> - fix the logic in qemu_add_wait_object() to avoid adding
>> >>   the same HANDLE twice
>> >>
>> >
>> > Still NACK, did you understand my argument about array bounds?
>> >
>> > "if (found)" will access the arrays at position i+1 == 
>> > MAXIMUM_WAIT_OBJECTS. We need the +1 for that logic to work without OOB 
>> > access.
>> >
>>
>> The delete logic was updated in v2. If position is at
>> MAXIMUM_WAIT_OBJECTS - 1, the loop will break.
>
>
> Ah I missed that. That new condition looks wrong to me. Not only it is 
> redundant with the loop condition check if w->num == MAXIMUM_WAIT_OBJECTS

Did you mean the "w->num >= MAXIMUM_WAIT_OBJECTS" check in
qemu_add_wait_object()? It's necessary because it prevents the OOB
access to the array.

> But you still access the array at MAXIMUM_WAIT_OBJECTS index, which requires 
> arrays of MAXIMUM_WAIT_OBJECTS+1 size, since it's 0-indexed..

There is no OOB access in either add or delete. Am I missing anything?

>
> Unless I say crap, which happens sometime :)
>

Regards,
Bin