* [Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes @ 2012-10-14 13:08 Tim Hardeck 2012-10-14 13:08 ` [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake Tim Hardeck 2012-10-14 13:08 ` [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Tim Hardeck 0 siblings, 2 replies; 16+ messages in thread From: Tim Hardeck @ 2012-10-14 13:08 UTC (permalink / raw) To: qemu-devel; +Cc: Tim Hardeck While trying to adapt Anthony Liguori's websockets patches to the current standard I ran into some segfaults. They were triggered during disconnects due to failed VNC handshakes. I have added checks to prevent them. Tim Hardeck (2): vnc: fix segfault due to failed handshake qemu queue: fix uninitialized removals qemu-queue.h | 8 ++++++-- ui/vnc.c | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) -- 1.7.10.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake 2012-10-14 13:08 [Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes Tim Hardeck @ 2012-10-14 13:08 ` Tim Hardeck 2012-10-17 12:52 ` Andreas Färber 2012-10-14 13:08 ` [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Tim Hardeck 1 sibling, 1 reply; 16+ messages in thread From: Tim Hardeck @ 2012-10-14 13:08 UTC (permalink / raw) To: qemu-devel; +Cc: 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) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake 2012-10-14 13:08 ` [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake Tim Hardeck @ 2012-10-17 12:52 ` Andreas Färber 0 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake @ 2012-10-17 12:52 ` Andreas Färber 0 siblings, 0 replies; 16+ 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] 16+ messages in thread
* [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-14 13:08 [Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes Tim Hardeck 2012-10-14 13:08 ` [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake Tim Hardeck @ 2012-10-14 13:08 ` Tim Hardeck 2012-10-17 15:00 ` Andreas Färber 2012-10-18 13:48 ` Peter Maydell 1 sibling, 2 replies; 16+ messages in thread From: Tim Hardeck @ 2012-10-14 13:08 UTC (permalink / raw) To: qemu-devel; +Cc: Tim Hardeck When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list QEMU segfaults. 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) \ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-14 13:08 ` [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Tim Hardeck @ 2012-10-17 15:00 ` Andreas Färber 2012-10-18 13:48 ` Peter Maydell 1 sibling, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals @ 2012-10-17 15:00 ` Andreas Färber 0 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-trivial] [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-17 15:00 ` Andreas Färber @ 2012-10-17 21:24 ` Tim Hardeck -1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals @ 2012-10-17 21:24 ` Tim Hardeck 0 siblings, 0 replies; 16+ 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] 16+ 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 -1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals @ 2012-10-18 10:43 ` Kevin Wolf 0 siblings, 0 replies; 16+ 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] 16+ 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 -1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals @ 2012-10-18 13:24 ` Andreas Färber 0 siblings, 0 replies; 16+ 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] 16+ 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:32 ` Peter Maydell -1 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals @ 2012-10-18 13:32 ` Peter Maydell 0 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals 2012-10-14 13:08 ` [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Tim Hardeck 2012-10-17 15:00 ` Andreas Färber @ 2012-10-18 13:48 ` Peter Maydell 1 sibling, 0 replies; 16+ messages in thread From: Peter Maydell @ 2012-10-18 13:48 UTC (permalink / raw) To: Tim Hardeck; +Cc: qemu-devel On 14 October 2012 14:08, Tim Hardeck <thardeck@suse.de> wrote: > When calling QTAILQ_REMOVE or QLIST_REMOVE on an unitialized list > QEMU segfaults. > > Check for this case specifically on item removal. Incidentally, this commit message is inaccurate -- you can't call the _REMOVE macros on a list (uninitialised or otherwise) because they take the list item, not the list itself. The case you are trying to guard against here is attempting to remove an item which never got inserted into the list in the first place. However this check doesn't catch all cases, because (a) there's no guarantee that the list element pointers get initialised to NULL and (b) removing an item from the list doesn't clear the pointers either, so this check still wouldn't catch "removed the item twice". Better just to accept that the semantics are "you can only use the _REMOVE macro on items that are actually in the list", I think. -- PMM ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-10-18 13:48 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-14 13:08 [Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes Tim Hardeck 2012-10-14 13:08 ` [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake Tim Hardeck 2012-10-17 12:52 ` [Qemu-trivial] " Andreas Färber 2012-10-17 12:52 ` Andreas Färber 2012-10-14 13:08 ` [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals Tim Hardeck 2012-10-17 15:00 ` [Qemu-trivial] " Andreas Färber 2012-10-17 15:00 ` Andreas Färber 2012-10-17 21:24 ` [Qemu-trivial] " Tim Hardeck 2012-10-17 21:24 ` Tim Hardeck 2012-10-18 10:43 ` [Qemu-trivial] " Kevin Wolf 2012-10-18 10:43 ` Kevin Wolf 2012-10-18 13:24 ` [Qemu-trivial] " Andreas Färber 2012-10-18 13:24 ` Andreas Färber 2012-10-18 13:32 ` [Qemu-trivial] " Peter Maydell 2012-10-18 13:32 ` Peter Maydell 2012-10-18 13:48 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.