* Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake [not found] ` <1350220128-10140-2-git-send-email-thardeck@suse.de> @ 2012-10-17 12:52 ` Andreas Färber 0 siblings, 0 replies; 6+ messages in thread From: Andreas Färber @ 2012-10-17 12:52 UTC (permalink / raw) To: Tim Hardeck; +Cc: qemu-trivial, qemu-devel, Anthony Liguori Am 14.10.2012 15:08, schrieb Tim Hardeck: > When the VNC server disconnects due to a failed handshake we don't have > vs->bh allocated yet. > > Check for this case and don't delete it. > > Signed-off-by: Tim Hardeck <thardeck@suse.de> > --- > ui/vnc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/ui/vnc.c b/ui/vnc.c > index 01b2daf..656895a 100644 > --- a/ui/vnc.c > +++ b/ui/vnc.c > @@ -1055,7 +1055,9 @@ static void vnc_disconnect_finish(VncState *vs) > vnc_unlock_output(vs); > > qemu_mutex_destroy(&vs->output_mutex); > - qemu_bh_delete(vs->bh); > + if (vs->bh != NULL) { > + qemu_bh_delete(vs->bh); > + } > buffer_free(&vs->jobs_buffer); > > for (i = 0; i < VNC_STAT_ROWS; ++i) { qemu_bh_delete() is not checking for a NULL argument, therefore this fix looks good to me, Acked-by: Andreas Färber <afaerber@suse.de> Adding some CCs. As a followup it might be a good idea to either assert or ignore a NULL argument in qemu_bh_delete(). Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <1350220128-10140-3-git-send-email-thardeck@suse.de>]
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals [not found] ` <1350220128-10140-3-git-send-email-thardeck@suse.de> @ 2012-10-17 15:00 ` Andreas Färber 2012-10-17 21:24 ` Tim Hardeck 0 siblings, 1 reply; 6+ messages in thread From: Andreas Färber @ 2012-10-17 15:00 UTC (permalink / raw) To: Tim Hardeck; +Cc: qemu-trivial, qemu-devel Tim, Am 14.10.2012 15:08, schrieb Tim Hardeck: > When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list > QEMU segfaults. Can this be reproduced by a user today? Or is this just fixing the case that a developer forgot to initialize a list? Regards, Andreas > Check for this case specifically on item removal. > > Signed-off-by: Tim Hardeck <thardeck@suse.de> > --- > qemu-queue.h | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/qemu-queue.h b/qemu-queue.h > index 9288cd8..47ed239 100644 > --- a/qemu-queue.h > +++ b/qemu-queue.h > @@ -141,7 +141,9 @@ struct { \ > if ((elm)->field.le_next != NULL) \ > (elm)->field.le_next->field.le_prev = \ > (elm)->field.le_prev; \ > - *(elm)->field.le_prev = (elm)->field.le_next; \ > + if ((elm)->field.le_prev != NULL) { \ > + *(elm)->field.le_prev = (elm)->field.le_next; \ > + } \ > } while (/*CONSTCOND*/0) > > #define QLIST_FOREACH(var, head, field) \ > @@ -381,7 +383,9 @@ struct { \ > (elm)->field.tqe_prev; \ > else \ > (head)->tqh_last = (elm)->field.tqe_prev; \ > - *(elm)->field.tqe_prev = (elm)->field.tqe_next; \ > + if ((elm)->field.tqe_prev != NULL) { \ > + *(elm)->field.tqe_prev = (elm)->field.tqe_next; \ > + } \ > } while (/*CONSTCOND*/0) > > #define QTAILQ_FOREACH(var, head, field) \ -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-17 15:00 ` [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Andreas Färber @ 2012-10-17 21:24 ` Tim Hardeck 2012-10-18 10:43 ` Kevin Wolf 0 siblings, 1 reply; 6+ messages in thread From: Tim Hardeck @ 2012-10-17 21:24 UTC (permalink / raw) To: Andreas Färber; +Cc: qemu-trivial, qemu-devel [-- Attachment #1: Type: text/plain, Size: 3039 bytes --] Hi Andreas, On Wednesday 17 October 2012 17:00:15 Andreas Färber wrote: > Tim, > > Am 14.10.2012 15:08, schrieb Tim Hardeck: > > When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list > > QEMU segfaults. > > Can this be reproduced by a user today? Or is this just fixing the case > that a developer forgot to initialize a list? I am not sure but in this case it happened during an early VNC connection state failure which most likely wouldn't happen to regular users. I triggered it while working on the VNC connection part. The issue could most likely be also fixed in the VNC connection initialization process but if this changes doesn't have a relevant performance impact they might prevent some other/future crashes. Regards Tim > > Regards, > Andreas > > > Check for this case specifically on item removal. > > > > Signed-off-by: Tim Hardeck <thardeck@suse.de> > > --- > > > > qemu-queue.h | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/qemu-queue.h b/qemu-queue.h > > index 9288cd8..47ed239 100644 > > --- a/qemu-queue.h > > +++ b/qemu-queue.h > > @@ -141,7 +141,9 @@ struct { > > \> > > if ((elm)->field.le_next != NULL) \ > > > > (elm)->field.le_next->field.le_prev = \ > > > > (elm)->field.le_prev; \ > > > > - *(elm)->field.le_prev = (elm)->field.le_next; \ > > + if ((elm)->field.le_prev != NULL) { \ > > + *(elm)->field.le_prev = (elm)->field.le_next; \ > > + } \ > > > > } while (/*CONSTCOND*/0) > > > > #define QLIST_FOREACH(var, head, field) \ > > > > @@ -381,7 +383,9 @@ struct { > > \> > > (elm)->field.tqe_prev; \ > > > > else \ > > > > (head)->tqh_last = (elm)->field.tqe_prev; \ > > > > - *(elm)->field.tqe_prev = (elm)->field.tqe_next; \ > > + if ((elm)->field.tqe_prev != NULL) { \ > > + *(elm)->field.tqe_prev = (elm)->field.tqe_next; \ > > + } \ > > > > } while (/*CONSTCOND*/0) > > > > #define QTAILQ_FOREACH(var, head, field) \ -- SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Maxfeldstr. 5, 90409 Nürnberg, Germany T: +49 (0) 911 74053-0 F: +49 (0) 911 74053-483 http://www.suse.de/ [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-17 21:24 ` Tim Hardeck @ 2012-10-18 10:43 ` Kevin Wolf 2012-10-18 13:24 ` Andreas Färber 2012-10-18 13:32 ` Peter Maydell 0 siblings, 2 replies; 6+ messages in thread From: Kevin Wolf @ 2012-10-18 10:43 UTC (permalink / raw) To: Tim Hardeck; +Cc: qemu-trivial, Andreas Färber, qemu-devel Am 17.10.2012 23:24, schrieb Tim Hardeck: > Hi Andreas, > > On Wednesday 17 October 2012 17:00:15 Andreas Färber wrote: >> Tim, >> >> Am 14.10.2012 15:08, schrieb Tim Hardeck: >>> When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list >>> QEMU segfaults. >> >> Can this be reproduced by a user today? Or is this just fixing the case >> that a developer forgot to initialize a list? > I am not sure but in this case it happened during an early VNC connection > state failure which most likely wouldn't happen to regular users. > I triggered it while working on the VNC connection part. > > The issue could most likely be also fixed in the VNC connection initialization > process but if this changes doesn't have a relevant performance impact they > might prevent some other/future crashes. At the same time, it could be hiding real bugs, where ignoring the QLIST_REMOVE() isn't the right fix. I can see your point, but I would be careful with making interfaces less strict. In any case, I don't think this qualifies for qemu-trivial, Andreas. Kevin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-18 10:43 ` Kevin Wolf @ 2012-10-18 13:24 ` Andreas Färber 2012-10-18 13:32 ` Peter Maydell 1 sibling, 0 replies; 6+ messages in thread From: Andreas Färber @ 2012-10-18 13:24 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-trivial, Tim Hardeck, qemu-devel Am 18.10.2012 12:43, schrieb Kevin Wolf: > Am 17.10.2012 23:24, schrieb Tim Hardeck: >> On Wednesday 17 October 2012 17:00:15 Andreas Färber wrote: >>> Am 14.10.2012 15:08, schrieb Tim Hardeck: >>>> When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list >>>> QEMU segfaults. >>> >>> Can this be reproduced by a user today? Or is this just fixing the case >>> that a developer forgot to initialize a list? >> I am not sure but in this case it happened during an early VNC connection >> state failure which most likely wouldn't happen to regular users. >> I triggered it while working on the VNC connection part. >> >> The issue could most likely be also fixed in the VNC connection initialization >> process but if this changes doesn't have a relevant performance impact they >> might prevent some other/future crashes. > > At the same time, it could be hiding real bugs, where ignoring the > QLIST_REMOVE() isn't the right fix. I can see your point, but I would be > careful with making interfaces less strict. What I don't get is, why is avoiding a NULL pointer dereference any better from accessing random memory through an uninitialized pointer? Or am I getting "uninitialized" wrong? > In any case, I don't think this qualifies for qemu-trivial, Andreas. Maybe not, but we don't have a clear maintainer that I'm aware of, and no one else reviewed it for several days before I did. ;) Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-18 10:43 ` Kevin Wolf 2012-10-18 13:24 ` Andreas Färber @ 2012-10-18 13:32 ` Peter Maydell 1 sibling, 0 replies; 6+ messages in thread From: Peter Maydell @ 2012-10-18 13:32 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-trivial, Tim Hardeck, Andreas Färber, qemu-devel On 18 October 2012 11:43, Kevin Wolf <kwolf@redhat.com> wrote: > Am 17.10.2012 23:24, schrieb Tim Hardeck: >> On Wednesday 17 October 2012 17:00:15 Andreas Färber wrote: >>> Am 14.10.2012 15:08, schrieb Tim Hardeck: >>>> When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list >>>> QEMU segfaults. >>> >>> Can this be reproduced by a user today? Or is this just fixing the case >>> that a developer forgot to initialize a list? >> I am not sure but in this case it happened during an early VNC connection >> state failure which most likely wouldn't happen to regular users. >> I triggered it while working on the VNC connection part. >> >> The issue could most likely be also fixed in the VNC connection initialization >> process but if this changes doesn't have a relevant performance impact they >> might prevent some other/future crashes. > > At the same time, it could be hiding real bugs, where ignoring the > QLIST_REMOVE() isn't the right fix. I can see your point, but I would be > careful with making interfaces less strict. I agree this patch doesn't seem like the right fix. All lists should be initialised (either via the _INIT macro or statically using the _HEAD_INITIALIZER macros) before use. If we ever try to do one of the other operations on an uninitialised list that's a bug which needs to be tracked down and fixed. -- PMM ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-18 13:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1350220128-10140-1-git-send-email-thardeck@suse.de>
[not found] ` <1350220128-10140-2-git-send-email-thardeck@suse.de>
2012-10-17 12:52 ` [Qemu-trivial] [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake Andreas Färber
[not found] ` <1350220128-10140-3-git-send-email-thardeck@suse.de>
2012-10-17 15:00 ` [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Andreas Färber
2012-10-17 21:24 ` Tim Hardeck
2012-10-18 10:43 ` Kevin Wolf
2012-10-18 13:24 ` Andreas Färber
2012-10-18 13:32 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).