[v2] fix bugs reported by Clang Static Analyzer

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

Posted by Philippe Mathieu-Daudé 8 years, 6 months ago

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é <f4bug@amsat.org>
---
 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 */
-    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. */
-- 
2.13.3

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

Posted by David Gibson 8 years, 6 months ago

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é <f4bug@amsat.org>

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.

> ---
>  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 */
> -    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. */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Posted by Philippe Mathieu-Daudé 8 years, 6 months ago

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é <f4bug@amsat.org>
> 
> 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. */
>