Patchwork vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

login
register
mail settings
Submitter Yoshiaki Tamura
Date Aug. 23, 2010, 12:55 a.m.
Message ID <1282524923-2368-1-git-send-email-tamura.yoshiaki@lab.ntt.co.jp>
Download mbox | patch
Permalink /patch/62420/
State New
Headers show

Comments

Yoshiaki Tamura - Aug. 23, 2010, 12:55 a.m.
Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
deleting.  This may cause a crash when a read handler calls
qemu_set_fd_handler2() to delete handlers, but a write handler is
still invoked from main_loop_wait().  Because main_loop_wait() checks
handlers before calling, setting NULL upon deleting will protect
handlers being called if already deleted.

One example is the new threaded vnc server.  When an error occurs in
the context of a read handler, it'll releases resources and deletes
handlers.  However, because the write handler still exists, it'll be
called, and then crashes because of lack of resources.  This patch
fixes it.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
---
 vl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Corentin Chary - Aug. 23, 2010, 6:19 a.m.
On Mon, Aug 23, 2010 at 2:55 AM, Yoshiaki Tamura
<tamura.yoshiaki@lab.ntt.co.jp> wrote:
> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
> deleting.  This may cause a crash when a read handler calls
> qemu_set_fd_handler2() to delete handlers, but a write handler is
> still invoked from main_loop_wait().  Because main_loop_wait() checks
> handlers before calling, setting NULL upon deleting will protect
> handlers being called if already deleted.
>
> One example is the new threaded vnc server.  When an error occurs in
> the context of a read handler, it'll releases resources and deletes
> handlers.  However, because the write handler still exists, it'll be
> called, and then crashes because of lack of resources.  This patch
> fixes it.
>
> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
> ---
>  vl.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index ccc8d57..7ae69ab 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -966,6 +966,8 @@ int qemu_set_fd_handler2(int fd,
>         QLIST_FOREACH(ioh, &io_handlers, next) {
>             if (ioh->fd == fd) {
>                 ioh->deleted = 1;
> +                ioh->fd_read = NULL;
> +                ioh->fd_write = NULL;
>                 break;
>             }
>         }
> --
> 1.7.1.1
>
>

Good catch, thanks,

Reviewed-by: Corentin Chary <corentincj@iksaif.net>
Yoshiaki Tamura - Sept. 6, 2010, 11:41 a.m.
2010/8/23 Corentin Chary <corentin.chary@gmail.com>:
> On Mon, Aug 23, 2010 at 2:55 AM, Yoshiaki Tamura
> <tamura.yoshiaki@lab.ntt.co.jp> wrote:
>> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
>> deleting.  This may cause a crash when a read handler calls
>> qemu_set_fd_handler2() to delete handlers, but a write handler is
>> still invoked from main_loop_wait().  Because main_loop_wait() checks
>> handlers before calling, setting NULL upon deleting will protect
>> handlers being called if already deleted.
>>
>> One example is the new threaded vnc server.  When an error occurs in
>> the context of a read handler, it'll releases resources and deletes
>> handlers.  However, because the write handler still exists, it'll be
>> called, and then crashes because of lack of resources.  This patch
>> fixes it.
>>
>> Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>> ---
>>  vl.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index ccc8d57..7ae69ab 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -966,6 +966,8 @@ int qemu_set_fd_handler2(int fd,
>>         QLIST_FOREACH(ioh, &io_handlers, next) {
>>             if (ioh->fd == fd) {
>>                 ioh->deleted = 1;
>> +                ioh->fd_read = NULL;
>> +                ioh->fd_write = NULL;
>>                 break;
>>             }
>>         }
>> --
>> 1.7.1.1
>>
>>
>
> Good catch, thanks,
>
> Reviewed-by: Corentin Chary <corentincj@iksaif.net>

Ping?

>
> --
> Corentin Chary
> http://xf.iksaif.net
>
>

Patch

diff --git a/vl.c b/vl.c
index ccc8d57..7ae69ab 100644
--- a/vl.c
+++ b/vl.c
@@ -966,6 +966,8 @@  int qemu_set_fd_handler2(int fd,
         QLIST_FOREACH(ioh, &io_handlers, next) {
             if (ioh->fd == fd) {
                 ioh->deleted = 1;
+                ioh->fd_read = NULL;
+                ioh->fd_write = NULL;
                 break;
             }
         }