[Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes

[Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes

[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

[Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake

[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

Re: [Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake

[Andreas Färber]

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

[Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[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

Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[Tim Hardeck]

[-- 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 --]

Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[Kevin Wolf]

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

Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 2/2] qemu queue: fix uninitialized removals

[Peter Maydell]

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

[Qemu-devel] [PATCH 0/2] fix segfaults triggered by failed vnc handshakes

[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

[Qemu-devel] [PATCH 1/2] vnc: fix segfault due to failed handshake

[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