Re: [Qemu-ppc] [PATCH for 2.10 v2 19/20] spapr_vio: fix overflow of qdevs in spapr_dt_vdevice()

[Philippe Mathieu-Daudé]

On 07/27/2017 12:43 AM, David Gibson wrote:
> On Wed, Jul 26, 2017 at 11:42:23PM -0300, Philippe Mathieu-Daudé wrote:
> > sizeof(ptr) was used instead of sizeof(struct)...
> >
> > also use g_malloc_n() which take care of possible type overflow.
> >
> > hw/ppc/spapr_vio.c:641:22: warning: The code calls sizeof() on a pointer type. 
> > This can produce an unexpected result
> >      qdevs = g_malloc(sizeof(qdev) * num);
> >                       ^     ~~~~~~
> > hw/ppc/spapr_vio.c:648:23: warning: The code calls sizeof() on a pointer type. 
> > This can produce an unexpected result
> >      qsort(qdevs, num, sizeof(qdev), compare_reg);
> >                        ^     ~~~~~~
> >
> > Reported-by: Clang Static Analyzer
> > Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>
> Nack.
>
> Have a closer look, what's going in the array really is pointers, not
> structures.  This is a false warning from clang, we need to find a
> different way to suppress it.

Something was bothering me with that patch, wondering why it never 
explode previously, now I see/understand.

> > ---
> >   hw/ppc/spapr_vio.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> > index ea3bc8bd9e..9991b44c9f 100644
> > --- a/hw/ppc/spapr_vio.c
> > +++ b/hw/ppc/spapr_vio.c
> > @@ -638,14 +638,14 @@ void spapr_dt_vdevice(VIOsPAPRBus *bus, void *fdt)
> >       }
> >   
> >       /* Copy out into an array of pointers */

/ashamed the comment was in front of me...

Thank you David for the review!

Phil.

> > -    qdevs = g_malloc(sizeof(qdev) * num);
> > +    qdevs = g_malloc_n(num, sizeof(*qdev));
> >       num = 0;
> >       QTAILQ_FOREACH(kid, &bus->bus.children, sibling) {
> >           qdevs[num++] = kid->child;
> >       }
> >   
> >       /* Sort the array */
> > -    qsort(qdevs, num, sizeof(qdev), compare_reg);
> > +    qsort(qdevs, num, sizeof(*qdev), compare_reg);
> >   
> >       /* Hack alert. Give the devices to libfdt in reverse order, we happen
> >        * to know that will mean they are in forward order in the tree. */