Patchwork [2/2] qemu queue: fix uninitialized removals

login
register
mail settings
Submitter Tim Hardeck
Date Oct. 14, 2012, 1:08 p.m.
Message ID <1350220128-10140-3-git-send-email-thardeck@suse.de>
Download mbox | patch
Permalink /patch/191348/
State New
Headers show

Comments

Tim Hardeck - Oct. 14, 2012, 1:08 p.m.
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(-)
Andreas Färber - Oct. 17, 2012, 3 p.m.
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)                                \
Tim Hardeck - Oct. 17, 2012, 9:24 p.m.
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)                                \
Kevin Wolf - Oct. 18, 2012, 10:43 a.m.
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
Andreas Färber - Oct. 18, 2012, 1:24 p.m.
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
Peter Maydell - Oct. 18, 2012, 1:32 p.m.
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
Peter Maydell - Oct. 18, 2012, 1:48 p.m.
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

Patch

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