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

[PATCH v3 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 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

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

[PATCH v3 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>
---

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

[PATCH v3 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>
---

(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 v3 2/3] util/main-loop: Avoid adding the same HANDLE twice

[Philippe Mathieu-Daudé via]

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>

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

[Philippe Mathieu-Daudé via]

On 24/8/22 10:52, 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>
> ---
> 
> (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é <f4bug@amsat.org>

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

[Bin Meng]

On Wed, Aug 24, 2022 at 4:52 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 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/util/main-loop.c b/util/main-loop.c
> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
>          if (w->events[i] == handle) {
>              found = 1;
>          }
> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> +            break;
> +        }
>          if (found) {
>              w->events[i] = w->events[i + 1];
>              w->func[i] = w->func[i + 1];
> --

Ping for this series?

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

[Bin Meng]

On Fri, Sep 2, 2022 at 12:19 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Aug 24, 2022 at 4:52 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 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 | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/util/main-loop.c b/util/main-loop.c
> > index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
> >          if (w->events[i] == handle) {
> >              found = 1;
> >          }
> > +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> > +            break;
> > +        }
> >          if (found) {
> >              w->events[i] = w->events[i + 1];
> >              w->func[i] = w->func[i + 1];
> > --
>
> Ping for this series?

Ping?

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

[Marc-André Lureau]

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

Hi

On Wed, Aug 24, 2022 at 12:52 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 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/util/main-loop.c b/util/main-loop.c
> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle,
> WaitObjectFunc *func, void *opaque)
>          if (w->events[i] == handle) {
>              found = 1;
>          }
> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> +            break;
> +        }
>

hmm


>          if (found) {
>              w->events[i] = w->events[i + 1];
>              w->func[i] = w->func[i + 1];
>

The way deletion works is by moving the i+1 element (which is always zeroed
for i == MAXIMUM_WAIT_OBJECTS) to i.

After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the
last value, and instead rely simply on updated w->num:

    if (found) {
        w->num--;
    }

 So your patch looks ok to me, but I prefer the current code.

Paolo, what do you say?

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

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

[Bin Meng]

Hi Paolo,

On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Wed, Aug 24, 2022 at 12:52 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 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 | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/util/main-loop.c b/util/main-loop.c
>> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
>>          if (w->events[i] == handle) {
>>              found = 1;
>>          }
>> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
>> +            break;
>> +        }
>
>
> hmm
>
>>
>>          if (found) {
>>              w->events[i] = w->events[i + 1];
>>              w->func[i] = w->func[i + 1];
>
>
> The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i.
>
> After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num:
>
>     if (found) {
>         w->num--;
>     }
>
>  So your patch looks ok to me, but I prefer the current code.
>
> Paolo, what do you say?

Ping?

Regards,
Bin

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

[Bin Meng]

Hi Paolo,

On Sun, Sep 25, 2022 at 9:07 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Paolo,
>
> On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
> >
> > Hi
> >
> > On Wed, Aug 24, 2022 at 12:52 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 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 | 11 +++++++----
> >>  1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/util/main-loop.c b/util/main-loop.c
> >> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
> >>          if (w->events[i] == handle) {
> >>              found = 1;
> >>          }
> >> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> >> +            break;
> >> +        }
> >
> >
> > hmm
> >
> >>
> >>          if (found) {
> >>              w->events[i] = w->events[i + 1];
> >>              w->func[i] = w->func[i + 1];
> >
> >
> > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i.
> >
> > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num:
> >
> >     if (found) {
> >         w->num--;
> >     }
> >
> >  So your patch looks ok to me, but I prefer the current code.
> >
> > Paolo, what do you say?
>
> Ping?
>

Ping?

Regards,
Bin

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

[Bin Meng]

+more people

On Mon, Oct 3, 2022 at 6:21 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Paolo,
>
> On Sun, Sep 25, 2022 at 9:07 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Paolo,
> >
> > On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau
> > <marcandre.lureau@gmail.com> wrote:
> > >
> > > Hi
> > >
> > > On Wed, Aug 24, 2022 at 12:52 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 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 | 11 +++++++----
> > >>  1 file changed, 7 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/util/main-loop.c b/util/main-loop.c
> > >> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
> > >>          if (w->events[i] == handle) {
> > >>              found = 1;
> > >>          }
> > >> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> > >> +            break;
> > >> +        }
> > >
> > >
> > > hmm
> > >
> > >>
> > >>          if (found) {
> > >>              w->events[i] = w->events[i + 1];
> > >>              w->func[i] = w->func[i + 1];
> > >
> > >
> > > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i.
> > >
> > > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num:
> > >
> > >     if (found) {
> > >         w->num--;
> > >     }
> > >
> > >  So your patch looks ok to me, but I prefer the current code.
> > >
> > > Paolo, what do you say?
> >
> > Ping?
> >
>
> Ping?
>

Could this series be merged? Thanks,

Regards,
Bin

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

[Bin Meng]

+Daniel,

On Tue, Oct 11, 2022 at 8:04 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> +more people
>
> On Mon, Oct 3, 2022 at 6:21 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Paolo,
> >
> > On Sun, Sep 25, 2022 at 9:07 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > Hi Paolo,
> > >
> > > On Tue, Sep 13, 2022 at 5:52 PM Marc-André Lureau
> > > <marcandre.lureau@gmail.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Wed, Aug 24, 2022 at 12:52 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 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 | 11 +++++++----
> > > >>  1 file changed, 7 insertions(+), 4 deletions(-)
> > > >>
> > > >> diff --git a/util/main-loop.c b/util/main-loop.c
> > > >> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
> > > >>          if (w->events[i] == handle) {
> > > >>              found = 1;
> > > >>          }
> > > >> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> > > >> +            break;
> > > >> +        }
> > > >
> > > >
> > > > hmm
> > > >
> > > >>
> > > >>          if (found) {
> > > >>              w->events[i] = w->events[i + 1];
> > > >>              w->func[i] = w->func[i + 1];
> > > >
> > > >
> > > > The way deletion works is by moving the i+1 element (which is always zeroed for i == MAXIMUM_WAIT_OBJECTS) to i.
> > > >
> > > > After your patch, for i == MAXIMUM_WAIT_OBJECTS, we no longer clear the last value, and instead rely simply on updated w->num:
> > > >
> > > >     if (found) {
> > > >         w->num--;
> > > >     }
> > > >
> > > >  So your patch looks ok to me, but I prefer the current code.
> > > >
> > > > Paolo, what do you say?
> > >
> > > Ping?
> > >
> >
> > Ping?
> >
>
> Could this series be merged? Thanks,
>

Since Polo keeps silent, Daniel would you help queue this series? Thanks!

Regards,
Bin

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

[Daniel P. Berrangé]

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
-- 
|: 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 v3 3/3] util/aio-win32: Correct the event array size in aio_poll()

[Daniel P. Berrangé]

On Wed, Aug 24, 2022 at 04:52:31PM +0800, 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>
> ---
> 
> (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];

Interestingly, the orignial + 1 was entirely pointless, since
the aio_poll impl has no bounds checking at all, until your
new assert.

>      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);
>          }
>      }

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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 v3 1/3] util/main-loop: Fix maximum number of wait objects for win32

[Daniel P. Berrangé]

On Wed, Aug 24, 2022 at 04:52:29PM +0800, 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 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/util/main-loop.c b/util/main-loop.c
> index f00a25451b..cb018dc33c 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,6 +395,9 @@ void qemu_del_wait_object(HANDLE handle, WaitObjectFunc *func, void *opaque)
>          if (w->events[i] == handle) {
>              found = 1;
>          }
> +        if (i == MAXIMUM_WAIT_OBJECTS - 1) {
> +            break;
> +        }

Took me a while to realize this was protecting the body
of the next if from out of bounds access. Can we redo
this to make it explicit:

>          if (found) {

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

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

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 v3 2/3] util/main-loop: Avoid adding the same HANDLE twice

[Bin Meng]

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