Patchwork [1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

login
register
mail settings
Submitter Corentin Chary
Date Jan. 25, 2011, 8:33 a.m.
Message ID <1295944407-19680-2-git-send-email-corentin.chary@gmail.com>
Download mbox | patch
Permalink /patch/80335/
State New
Headers show

Comments

Corentin Chary - Jan. 25, 2011, 8:33 a.m.
From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>

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>
Reviewed-by: Corentin Chary <corentincj@iksaif.net>
---
 vl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Stefan Hajnoczi - Jan. 25, 2011, 10:03 a.m.
On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
<corentin.chary@gmail.com> wrote:
> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>
> 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.

Does this case still happen with qemu.git/master?  In November I sent
a patch to check for deleted handlers:

commit 0290b57bdfec83ca78b6d119ea9847bb17943328
Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Date:   Wed Nov 3 14:29:44 2010 +0000

    Delete IOHandlers after potentially running them

    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
    handler that deletes its IOHandler is exposed to .fd_write() being
    called on the deleted IOHandler.

    This patch fixes deletion so that .fd_read() and .fd_write() are never
    called on an IOHandler that is marked for deletion.

    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

So I don't think Yoshi's patch is necessary anymore?

Stefan
Corentin Chary - Jan. 25, 2011, 10:13 a.m.
On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
> <corentin.chary@gmail.com> wrote:
>> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>
>> 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.
>
> Does this case still happen with qemu.git/master?  In November I sent
> a patch to check for deleted handlers:
>
> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
> Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Date:   Wed Nov 3 14:29:44 2010 +0000
>
>    Delete IOHandlers after potentially running them
>
>    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>    handler that deletes its IOHandler is exposed to .fd_write() being
>    called on the deleted IOHandler.
>
>    This patch fixes deletion so that .fd_read() and .fd_write() are never
>    called on an IOHandler that is marked for deletion.
>
>    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> So I don't think Yoshi's patch is necessary anymore?

Ho I didn't see that one.
It's probably not necessary, but it stills make sense to apply this
patch since there is
absolutly no reasons to keep the old value in fd_read and fd_write when
the user explicitly asked to set them to NULL.
Stefan Hajnoczi - Jan. 25, 2011, 10:26 a.m.
On Tue, Jan 25, 2011 at 10:13 AM, Corentin Chary
<corentin.chary@gmail.com> wrote:
> On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
>> <corentin.chary@gmail.com> wrote:
>>> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>>
>>> 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.
>>
>> Does this case still happen with qemu.git/master?  In November I sent
>> a patch to check for deleted handlers:
>>
>> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
>> Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> Date:   Wed Nov 3 14:29:44 2010 +0000
>>
>>    Delete IOHandlers after potentially running them
>>
>>    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>>    handler that deletes its IOHandler is exposed to .fd_write() being
>>    called on the deleted IOHandler.
>>
>>    This patch fixes deletion so that .fd_read() and .fd_write() are never
>>    called on an IOHandler that is marked for deletion.
>>
>>    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> So I don't think Yoshi's patch is necessary anymore?
>
> Ho I didn't see that one.
> It's probably not necessary, but it stills make sense to apply this
> patch since there is
> absolutly no reasons to keep the old value in fd_read and fd_write when
> the user explicitly asked to set them to NULL.

That's true, I don't see a good reason why we shouldn't clear them.
The only minor advantage to keeping them is that it helps when
debugging QEMU - you can identify the fd handler by its
fd_read/fd_write function pointers easily.

Stefan
Yoshiaki Tamura - Jan. 25, 2011, 12:05 p.m.
2011/1/25 Stefan Hajnoczi <stefanha@gmail.com>:
> On Tue, Jan 25, 2011 at 10:13 AM, Corentin Chary
> <corentin.chary@gmail.com> wrote:
>> On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
>>> <corentin.chary@gmail.com> wrote:
>>>> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>>>
>>>> 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.
>>>
>>> Does this case still happen with qemu.git/master?  In November I sent
>>> a patch to check for deleted handlers:
>>>
>>> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
>>> Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>> Date:   Wed Nov 3 14:29:44 2010 +0000
>>>
>>>    Delete IOHandlers after potentially running them
>>>
>>>    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>>>    handler that deletes its IOHandler is exposed to .fd_write() being
>>>    called on the deleted IOHandler.
>>>
>>>    This patch fixes deletion so that .fd_read() and .fd_write() are never
>>>    called on an IOHandler that is marked for deletion.
>>>
>>>    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>
>>> So I don't think Yoshi's patch is necessary anymore?
>>
>> Ho I didn't see that one.
>> It's probably not necessary, but it stills make sense to apply this
>> patch since there is
>> absolutly no reasons to keep the old value in fd_read and fd_write when
>> the user explicitly asked to set them to NULL.
>
> That's true, I don't see a good reason why we shouldn't clear them.
> The only minor advantage to keeping them is that it helps when
> debugging QEMU - you can identify the fd handler by its
> fd_read/fd_write function pointers easily.

Well, since I posted the patch in August, I won't get surprised
even the bug might be fixed due to other patches :)  Regardless of
that, I don't see the patch is doing something incorrect either.
If some codes were relying on unset values, that should be wrong.

Yoshi

>
> Stefan
>
>

Patch

diff --git a/vl.c b/vl.c
index 14255c4..7a26bea 100644
--- a/vl.c
+++ b/vl.c
@@ -1037,6 +1037,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;
             }
         }