[PATCH v4 1/3] util/main-loop: Fix maximum number of wait objects for win32

[PATCH v4 1/3] util/main-loop: Fix maximum number of wait objects for win32

[Bin Meng]

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.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

Changes in v4:
- make the out of bounds access protection explicit

Changes in v3:
- move the check of adding the same HANDLE twice to a separete patch

Changes in v2:
- fix the logic in qemu_add_wait_object() to avoid adding
  the same HANDLE twice

 util/main-loop.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/main-loop.c b/util/main-loop.c
index f00a25451b..de38876064 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -363,10 +363,10 @@ 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};
@@ -395,7 +395,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
         if (w->events[i] == handle) {
             found = 1;
         }
-        if (found) {
+        if (found && i < (MAXIMUM_WAIT_OBJECTS - 1)) {
             w->events[i] = w->events[i + 1];
             w->func[i] = w->func[i + 1];
             w->opaque[i] = w->opaque[i + 1];
-- 
2.34.1

[PATCH v4 2/3] util/main-loop: Avoid adding the same HANDLE twice

[Bin Meng]

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>
---

(no changes since v3)

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 aac707d073..3c9a9a982d 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 de38876064..10fa74c6e3 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

Re: [PATCH v4 2/3] util/main-loop: Avoid adding the same HANDLE twice

[Philippe Mathieu-Daudé]

On 19/10/22 12:20, 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>
> ---
> 
> (no changes since v3)
> 
> 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é <philmd@linaro.org>

[PATCH v4 3/3] util/aio-win32: Correct the event array size in aio_poll()

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
object handles. Correct the event array size in aio_poll() and
add a assert() to ensure it does not cause out of bound access.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---

(no changes since v2)

Changes in v2:
- change 'count' to unsigned

 util/aio-win32.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..80cfe012ad 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,9 +326,9 @@ void aio_dispatch(AioContext *ctx)
 bool aio_poll(AioContext *ctx, bool blocking)
 {
     AioHandler *node;
-    HANDLE events[MAXIMUM_WAIT_OBJECTS + 1];
+    HANDLE events[MAXIMUM_WAIT_OBJECTS];
     bool progress, have_select_revents, first;
-    int count;
+    unsigned count;
     int timeout;
 
     /*
@@ -369,6 +369,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
     QLIST_FOREACH_RCU(node, &ctx->aio_handlers, node) {
         if (!node->deleted && node->io_notify
             && aio_node_check(ctx, node->is_external)) {
+            assert(count < MAXIMUM_WAIT_OBJECTS);
             events[count++] = event_notifier_get_handle(node->e);
         }
     }
-- 
2.34.1

Re: [PATCH v4 3/3] util/aio-win32: Correct the event array size in aio_poll()

[Philippe Mathieu-Daudé]

On 19/10/22 12:20, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> WaitForMultipleObjects() can only wait for MAXIMUM_WAIT_OBJECTS
> object handles. Correct the event array size in aio_poll() and
> add a assert() to ensure it does not cause out of bound access.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> Reviewed-by: Stefan Weil <sw@weilnetz.de>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
> - change 'count' to unsigned
> 
>   util/aio-win32.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Bin Meng]

On Wed, Oct 19, 2022 at 6:20 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.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
> Changes in v4:
> - make the out of bounds access protection explicit
>
> Changes in v3:
> - move the check of adding the same HANDLE twice to a separete patch
>
> Changes in v2:
> - fix the logic in qemu_add_wait_object() to avoid adding
>   the same HANDLE twice
>
>  util/main-loop.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>

Ping?

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

[Bin Meng]

Hi Daniel,

On Wed, Oct 26, 2022 at 12:41 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Oct 19, 2022 at 6:20 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.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> > Changes in v4:
> > - make the out of bounds access protection explicit
> >
> > Changes in v3:
> > - move the check of adding the same HANDLE twice to a separete patch
> >
> > Changes in v2:
> > - fix the logic in qemu_add_wait_object() to avoid adding
> >   the same HANDLE twice
> >
> >  util/main-loop.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
>
> Ping?

Would you queue this series? Thanks!

Regards,
Bin

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

[Daniel P. Berrangé]

On Tue, Nov 01, 2022 at 09:14:55AM +0800, Bin Meng wrote:
> Hi Daniel,
> 
> On Wed, Oct 26, 2022 at 12:41 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Wed, Oct 19, 2022 at 6:20 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.
> > >
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > ---
> > >
> > > Changes in v4:
> > > - make the out of bounds access protection explicit
> > >
> > > Changes in v3:
> > > - move the check of adding the same HANDLE twice to a separete patch
> > >
> > > Changes in v2:
> > > - fix the logic in qemu_add_wait_object() to avoid adding
> > >   the same HANDLE twice
> > >
> > >  util/main-loop.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> >
> > Ping?
> 
> Would you queue this series? Thanks!

The main loop is not my area as maintainer - it would normally be
Paolo IIRC.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Bin Meng]

On Tue, Nov 1, 2022 at 8:03 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Nov 01, 2022 at 09:14:55AM +0800, Bin Meng wrote:
> > Hi Daniel,
> >
> > On Wed, Oct 26, 2022 at 12:41 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Wed, Oct 19, 2022 at 6:20 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.
> > > >
> > > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > > ---
> > > >
> > > > Changes in v4:
> > > > - make the out of bounds access protection explicit
> > > >
> > > > Changes in v3:
> > > > - move the check of adding the same HANDLE twice to a separete patch
> > > >
> > > > Changes in v2:
> > > > - fix the logic in qemu_add_wait_object() to avoid adding
> > > >   the same HANDLE twice
> > > >
> > > >  util/main-loop.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > >
> > > Ping?
> >
> > Would you queue this series? Thanks!
>
> The main loop is not my area as maintainer - it would normally be
> Paolo IIRC.
>

Thanks, but Paolo has been silent since day 1 ...

Regards,
Bin

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

[Philippe Mathieu-Daudé]

On 19/10/22 12:20, Bin Meng 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.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
> Changes in v4:
> - make the out of bounds access protection explicit
> 
> Changes in v3:
> - move the check of adding the same HANDLE twice to a separete patch
> 
> Changes in v2:
> - fix the logic in qemu_add_wait_object() to avoid adding
>    the same HANDLE twice
> 
>   util/main-loop.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/util/main-loop.c b/util/main-loop.c
> index f00a25451b..de38876064 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -363,10 +363,10 @@ 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};
> @@ -395,7 +395,7 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
>           if (w->events[i] == handle) {
>               found = 1;
>           }
> -        if (found) {
> +        if (found && i < (MAXIMUM_WAIT_OBJECTS - 1)) {

Matter of style, I find this form easier to review (same logic than
what follows):

            if (found && i + 1 < MAXIMUM_WAIT_OBJECTS) {

>               w->events[i] = w->events[i + 1];
>               w->func[i] = w->func[i + 1];
>               w->opaque[i] = w->opaque[i + 1];

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>