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

[PATCH 1/2] 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>
---

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

diff --git a/util/main-loop.c b/util/main-loop.c
index f00a25451b..f15d8e7d12 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};
-- 
2.34.1

[PATCH 2/2] 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>
---

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

diff --git a/util/aio-win32.c b/util/aio-win32.c
index 44003d645e..8cf5779567 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -326,7 +326,7 @@ 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;
     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 2/2] util/aio-win32: Correct the event array size in aio_poll()

[Stefan Weil via]

Am 05.08.22 um 16:56 schrieb 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>
> ---
>
>   util/aio-win32.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index 44003d645e..8cf5779567 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -326,7 +326,7 @@ 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;
>       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);


Would using g_assert for new code be better? Currently the rest of that 
file (and most QEMU code) uses assert.

count could also be changed from int to unsigned (which matches better 
to the unsigned DWORD).

Reviewed-by: Stefan Weil <sw@weilnetz.de>

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

[Bin Meng]

On Fri, Aug 5, 2022 at 11:09 PM Stefan Weil <sw@weilnetz.de> wrote:
>
> Am 05.08.22 um 16:56 schrieb 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>
> > ---
> >
> >   util/aio-win32.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/aio-win32.c b/util/aio-win32.c
> > index 44003d645e..8cf5779567 100644
> > --- a/util/aio-win32.c
> > +++ b/util/aio-win32.c
> > @@ -326,7 +326,7 @@ 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;
> >       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);
>
>
> Would using g_assert for new code be better? Currently the rest of that
> file (and most QEMU code) uses assert.

Yeah, I noticed that but didn't do that because I feel it's better to
be consistent, at least in this single file.

Changing to g_assert() could be a future patch, if necessary.

>
> count could also be changed from int to unsigned (which matches better
> to the unsigned DWORD).
>

changed in v2.

> Reviewed-by: Stefan Weil <sw@weilnetz.de>

Thanks!

Regards,
Bin

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

[Bin Meng]

On Fri, Aug 5, 2022 at 10:56 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>
> ---
>
>  util/main-loop.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>

Ping?

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

[Philippe Mathieu-Daudé via]

On Mon, Aug 8, 2022 at 5:56 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Fri, Aug 5, 2022 at 10:56 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>
> > ---
> >
> >  util/main-loop.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
>
> Ping?

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

NB: qemu_del_wait_object() seems dubious in case the same handle is
added more than once with qemu_add_wait_object().

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1542 bytes --]

Hi

On Fri, Aug 5, 2022 at 6:57 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>
>

Nack,

if wait_objects.num reaches MAXIMUM_WAIT_OBJECTS,

then qemu_del_wait_object() will iterate up to it, and then the branch "if
(found)" will access the arrays at position i+1 == MAXIMUM_WAIT_OBJECTS.

Note that the add functions should probably learn to avoid adding the same
HANDLE twice, otherwise del is a bit broken.

---
>
>  util/main-loop.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/util/main-loop.c b/util/main-loop.c
> index f00a25451b..f15d8e7d12 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};
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 2403 bytes --]

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

[Bin Meng]

On Tue, Aug 9, 2022 at 9:15 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Fri, Aug 5, 2022 at 6:57 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>
>
>
> Nack,
>
> if wait_objects.num reaches MAXIMUM_WAIT_OBJECTS,
>
> then qemu_del_wait_object() will iterate up to it, and then the branch "if (found)" will access the arrays at position i+1 == MAXIMUM_WAIT_OBJECTS.
>
> Note that the add functions should probably learn to avoid adding the same HANDLE twice, otherwise del is a bit broken.
>

Thanks for the review. Will fix in v2.

Regards,
Bin