| Message ID | 20220809164308.1182645-1-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] util/main-loop: Fix maximum number of wait objects for win32 | expand |
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. > util/main-loop.c | 43 +++++++++++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 12 deletions(-) > > diff --git a/util/main-loop.c b/util/main-loop.c > index f00a25451b..66b2ae2800 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -363,37 +363,56 @@ void qemu_del_polling_cb(PollingFunc *func, void > *opaque) > /* Wait objects support */ > typedef struct WaitObjects { > int num; > - int revents[MAXIMUM_WAIT_OBJECTS + 1]; > - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; > - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; > - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; > + int revents[MAXIMUM_WAIT_OBJECTS]; > + HANDLE events[MAXIMUM_WAIT_OBJECTS]; > + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; > + void *opaque[MAXIMUM_WAIT_OBJECTS]; > } WaitObjects; > > static WaitObjects wait_objects = {0}; > > int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void > *opaque) > { > + int i; > + bool found = false; > WaitObjects *w = &wait_objects; > + > if (w->num >= MAXIMUM_WAIT_OBJECTS) { > return -1; > } > - w->events[w->num] = handle; > - w->func[w->num] = func; > - w->opaque[w->num] = opaque; > - w->revents[w->num] = 0; > - w->num++; > + > + for (i = 0; i < w->num; i++) { > + /* if the same handle is added twice, newer overwrites older */ > + if (w->events[i] == handle) { > + found = true; > + break; > + } > + } > + > + w->events[i] = handle; > + w->func[i] = func; > + w->opaque[i] = opaque; > + w->revents[i] = 0; > + > + if (!found) { > + w->num++; > + } > + > return 0; > } > > void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void > *opaque) > { > - int i, found; > + int i; > + bool found = false; > WaitObjects *w = &wait_objects; > > - found = 0; > for (i = 0; i < w->num; i++) { > if (w->events[i] == handle) { > - found = 1; > + found = true; > + } > + if (i == MAXIMUM_WAIT_OBJECTS - 1) { > + break; > } > if (found) { > w->events[i] = w->events[i + 1]; > -- > 2.34.1 > >
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. Regards, Bin
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 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.. Unless I say crap, which happens sometime :)
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
diff --git a/util/main-loop.c b/util/main-loop.c index f00a25451b..66b2ae2800 100644 --- a/util/main-loop.c +++ b/util/main-loop.c @@ -363,37 +363,56 @@ void qemu_del_polling_cb(PollingFunc *func, void *opaque) /* Wait objects support */ typedef struct WaitObjects { int num; - int revents[MAXIMUM_WAIT_OBJECTS + 1]; - HANDLE events[MAXIMUM_WAIT_OBJECTS + 1]; - WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS + 1]; - void *opaque[MAXIMUM_WAIT_OBJECTS + 1]; + int revents[MAXIMUM_WAIT_OBJECTS]; + HANDLE events[MAXIMUM_WAIT_OBJECTS]; + WaitObjectFunc *func[MAXIMUM_WAIT_OBJECTS]; + void *opaque[MAXIMUM_WAIT_OBJECTS]; } WaitObjects; static WaitObjects wait_objects = {0}; int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) { + int i; + bool found = false; WaitObjects *w = &wait_objects; + if (w->num >= MAXIMUM_WAIT_OBJECTS) { return -1; } - w->events[w->num] = handle; - w->func[w->num] = func; - w->opaque[w->num] = opaque; - w->revents[w->num] = 0; - w->num++; + + for (i = 0; i < w->num; i++) { + /* if the same handle is added twice, newer overwrites older */ + if (w->events[i] == handle) { + found = true; + break; + } + } + + w->events[i] = handle; + w->func[i] = func; + w->opaque[i] = opaque; + w->revents[i] = 0; + + if (!found) { + w->num++; + } + return 0; } void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) { - int i, found; + int i; + bool found = false; WaitObjects *w = &wait_objects; - found = 0; for (i = 0; i < w->num; i++) { if (w->events[i] == handle) { - found = 1; + found = true; + } + if (i == MAXIMUM_WAIT_OBJECTS - 1) { + break; } if (found) { w->events[i] = w->events[i + 1];