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

Submitted by Corentin Chary on Jan. 25, 2011, 8:33 a.m.

Details

Message ID 1295944407-19680-2-git-send-email-corentin.chary@gmail.com
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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