| Message ID | 1295944407-19680-2-git-send-email-corentin.chary@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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 > >
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; } }