| Message ID | 20220824085231.1630804-2-bmeng.cn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] util/main-loop: Fix maximum number of wait objects for win32 | expand |
On 24/8/22 10:52, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > Fix the logic in qemu_add_wait_object() to avoid adding the same > HANDLE twice, as the behavior is undefined when passing an array > that contains same HANDLEs to WaitForMultipleObjects() API. > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v3: > - new patch: avoid adding the same HANDLE twice > > include/qemu/main-loop.h | 2 ++ > util/main-loop.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On Wed, Aug 24, 2022 at 04:52:30PM +0800, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > Fix the logic in qemu_add_wait_object() to avoid adding the same > HANDLE twice, as the behavior is undefined when passing an array > that contains same HANDLEs to WaitForMultipleObjects() API. Have you encountered this problem in the real world, or is this just a flaw you spotted through code inspection ? Essentially I'm wondering if there's any known caller that is making this mistake of adding it twice ? > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > --- > > Changes in v3: > - new patch: avoid adding the same HANDLE twice > > include/qemu/main-loop.h | 2 ++ > util/main-loop.c | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h > index c50d1b7e3a..db8d380550 100644 > --- a/include/qemu/main-loop.h > +++ b/include/qemu/main-loop.h > @@ -157,6 +157,8 @@ typedef void WaitObjectFunc(void *opaque); > * in the main loop's calls to WaitForMultipleObjects. When the handle > * is in a signaled state, QEMU will call @func. > * > + * If the same HANDLE is added twice, this function returns -1. > + * > * @handle: The Windows handle to be observed. > * @func: A function to be called when @handle is in a signaled state. > * @opaque: A pointer-size value that is passed to @func. > diff --git a/util/main-loop.c b/util/main-loop.c > index cb018dc33c..dae33a8daf 100644 > --- a/util/main-loop.c > +++ b/util/main-loop.c > @@ -373,10 +373,20 @@ static WaitObjects wait_objects = {0}; > > int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) > { > + int i; > WaitObjects *w = &wait_objects; > + > if (w->num >= MAXIMUM_WAIT_OBJECTS) { > return -1; > } > + > + for (i = 0; i < w->num; i++) { > + /* check if the same handle is added twice */ > + if (w->events[i] == handle) { > + return -1; > + } > + } > + > w->events[w->num] = handle; > w->func[w->num] = func; > w->opaque[w->num] = opaque; > -- > 2.34.1 > > With regards, Daniel
Hi Daniel, On Wed, Oct 19, 2022 at 4:32 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Aug 24, 2022 at 04:52:30PM +0800, Bin Meng wrote: > > From: Bin Meng <bin.meng@windriver.com> > > > > Fix the logic in qemu_add_wait_object() to avoid adding the same > > HANDLE twice, as the behavior is undefined when passing an array > > that contains same HANDLEs to WaitForMultipleObjects() API. > > Have you encountered this problem in the real world, or is this > just a flaw you spotted through code inspection ? No. This was noticed as part of debugging [1] and code inspection was done for all possible suspicious places. [1] https://lore.kernel.org/qemu-devel/20221006151927.2079583-17-bmeng.cn@gmail.com/ > > Essentially I'm wondering if there's any known caller that is > making this mistake of adding it twice ? No known caller at this call chain. But there is another in the QIO socket channel APIs that may add the same handle twice, fortunately that scenario is handled properly in the GLib internally. > > > > > Signed-off-by: Bin Meng <bin.meng@windriver.com> > > --- > > > > Changes in v3: > > - new patch: avoid adding the same HANDLE twice > > > > include/qemu/main-loop.h | 2 ++ > > util/main-loop.c | 10 ++++++++++ > > 2 files changed, 12 insertions(+) > > Regards, Bin
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h index c50d1b7e3a..db8d380550 100644 --- a/include/qemu/main-loop.h +++ b/include/qemu/main-loop.h @@ -157,6 +157,8 @@ typedef void WaitObjectFunc(void *opaque); * in the main loop's calls to WaitForMultipleObjects. When the handle * is in a signaled state, QEMU will call @func. * + * If the same HANDLE is added twice, this function returns -1. + * * @handle: The Windows handle to be observed. * @func: A function to be called when @handle is in a signaled state. * @opaque: A pointer-size value that is passed to @func. diff --git a/util/main-loop.c b/util/main-loop.c index cb018dc33c..dae33a8daf 100644 --- a/util/main-loop.c +++ b/util/main-loop.c @@ -373,10 +373,20 @@ static WaitObjects wait_objects = {0}; int qemu_add_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque) { + int i; WaitObjects *w = &wait_objects; + if (w->num >= MAXIMUM_WAIT_OBJECTS) { return -1; } + + for (i = 0; i < w->num; i++) { + /* check if the same handle is added twice */ + if (w->events[i] == handle) { + return -1; + } + } + w->events[w->num] = handle; w->func[w->num] = func; w->opaque[w->num] = opaque;