Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-11 Thread Peter Krempa 
On Wed, Jun 10, 2015 at 15:18:15 -0600, Eric Blake wrote:
 On 06/10/2015 09:27 AM, John Ferlan wrote:
 
  So there are basically three options:
  1) Silence the coverity warning
  
  So on line just prior to CPU_ISSET_S either:
  
  sa_assert(sizeof(unsigned long int) == sizeof(cpu_set_t));
 
 Is that true even on 32-bit platforms?

This is never true. cpu_set_t is a structure that contains array of
unsigned longs that is 1024bits wide.

 
  
  or
  
  /* coverity[overrun-local] */
 
 Might be safer, even though I generally prefer sa_assert() over /* magic
 comment */

I'd go with this one in this case since the sa_assert one above probably
silences the warning in a bogous way.



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-10 Thread Peter Krempa 
On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
 On 06/04/2015 08:15 AM, Peter Krempa wrote:
  Refactor the code flow a bit more to clear coverity errors.
  
  Store the cpu count in an intermediate variable and reuse it rather than
  caluclating the index.
  ---
   src/util/virprocess.c | 11 +++
   1 file changed, 7 insertions(+), 4 deletions(-)
  

...

 
 ^^^ Coverity still complains here
 
 No real change since previous...
 

So I went in and traced it a bit. cpu_set_t is basically the following
structure: (assuming sizeof(unsigned long int) == 8))

typedef struct
{
  unsigned long int __bits[1024 / 8 * 8];
} cpu_set_t;


The CPU_ALLOC() macro allocates an array of such structures. Since the
size of the __bits array is rounded nicely (128 bytes) the structure
itself does not add any padding and it's size is 128 bytes too the
structures are packed and thus the __bits fields of adjecent structures
basically form a longer array of unsigned long ints.

The CPU_ISSET_S macro then translates basically to this equivalent
function:

unsigned long int
CPU_ISSET_S(size_t __cpu,
size_t setsize,
cpu_set_t *cpusetp)
{
if (__cpu / 8  setsize) {
unsigned long int subcpu =  ((const unsigned long int 
*)(mask-__bits))[__cpu / 64];
unsigned long int mask = (unsigned long int) 1  (__cpu % 64);

return subcpu  mask;
} else {
return 0;
}
}

The macro expects that the array is packed nicely and treats it
basically as a large array of unsigned ints. The macro then selects one
of the members and masks out the required cpu bit.

So then compiling the following proof of concept:

#define _GNU_SOURCE
#include sched.h

int main(void)
{
size_t ncpus = 1024  8;
size_t masklen = CPU_ALLOC_SIZE(ncpus);
cpu_set_t *mask = CPU_ALLOC(ncpus);

CPU_ZERO_S(masklen, mask);

for (size_t i = 0; i  ncpus; i++) {
if (CPU_ISSET_S(i, masklen, mask)) {
i = i;
}
}
CPU_FREE(mask);

return 0;
}

And running it in valgrind results in no error as expected:

$ valgrind ./a.out 
==95609== Memcheck, a memory error detector
==95609== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==95609== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==95609== Command: ./a.out
==95609== 
==95609== 
==95609== HEAP SUMMARY:
==95609== in use at exit: 0 bytes in 0 blocks
==95609==   total heap usage: 1 allocs, 1 frees, 32,768 bytes allocated
==95609== 
==95609== All heap blocks were freed -- no leaks are possible
==95609== 
==95609== For counts of detected and suppressed errors, rerun with: -v
==95609== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The problem is that coverity thinks that the code overruns the __bits
array now that the code is simple enough to introspect which is
technically true. Previously the same situation would happen but the
loop that would resize the array incrementally probably was not
introspected properly and thus did not produce a warning.

So there are basically three options:
1) Silence the coverity warning
2) File a bug against libc or something to fix the macro
3) Reimplement CPU_ISSET_S internally. (Which I don't think will be
possible since cpu_set_t does not look public)

At any rate, there is no problem in libvirt's usage as it conforms to
the example in the man page.

As of this particular patch. It does not fix the coverity warning, but I
think the intention and code flow is more apparent once it's applied.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-10 Thread John Ferlan 


On 06/10/2015 09:41 AM, Peter Krempa wrote:
 On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
 On 06/04/2015 08:15 AM, Peter Krempa wrote:
 Refactor the code flow a bit more to clear coverity errors.

 Store the cpu count in an intermediate variable and reuse it rather than
 caluclating the index.
 ---
  src/util/virprocess.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

 
 ...
 

 ^^^ Coverity still complains here

 No real change since previous...


nice detective work...

 
 So I went in and traced it a bit. cpu_set_t is basically the following
 structure: (assuming sizeof(unsigned long int) == 8))
 
 typedef struct
 {
   unsigned long int __bits[1024 / 8 * 8];
 } cpu_set_t;
 
 
 The CPU_ALLOC() macro allocates an array of such structures. Since the
 size of the __bits array is rounded nicely (128 bytes) the structure
 itself does not add any padding and it's size is 128 bytes too the
 structures are packed and thus the __bits fields of adjecent structures
 basically form a longer array of unsigned long ints.
 
 The CPU_ISSET_S macro then translates basically to this equivalent
 function:
 
 unsigned long int
 CPU_ISSET_S(size_t __cpu,
 size_t setsize,
 cpu_set_t *cpusetp)
 {
 if (__cpu / 8  setsize) {
 unsigned long int subcpu =  ((const unsigned long int 
 *)(mask-__bits))[__cpu / 64];
 unsigned long int mask = (unsigned long int) 1  (__cpu % 64);
 
 return subcpu  mask;
 } else {
 return 0;
 }
 }
 
 The macro expects that the array is packed nicely and treats it
 basically as a large array of unsigned ints. The macro then selects one
 of the members and masks out the required cpu bit.
 
 So then compiling the following proof of concept:
 
 #define _GNU_SOURCE
 #include sched.h
 
 int main(void)
 {
 size_t ncpus = 1024  8;
 size_t masklen = CPU_ALLOC_SIZE(ncpus);
 cpu_set_t *mask = CPU_ALLOC(ncpus);
 
 CPU_ZERO_S(masklen, mask);
 
 for (size_t i = 0; i  ncpus; i++) {
 if (CPU_ISSET_S(i, masklen, mask)) {
 i = i;
 }
 }
 CPU_FREE(mask);
 
 return 0;
 }
 
 And running it in valgrind results in no error as expected:
 
 $ valgrind ./a.out 
 ==95609== Memcheck, a memory error detector
 ==95609== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
 ==95609== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
 ==95609== Command: ./a.out
 ==95609== 
 ==95609== 
 ==95609== HEAP SUMMARY:
 ==95609== in use at exit: 0 bytes in 0 blocks
 ==95609==   total heap usage: 1 allocs, 1 frees, 32,768 bytes allocated
 ==95609== 
 ==95609== All heap blocks were freed -- no leaks are possible
 ==95609== 
 ==95609== For counts of detected and suppressed errors, rerun with: -v
 ==95609== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
 
 The problem is that coverity thinks that the code overruns the __bits
 array now that the code is simple enough to introspect which is
 technically true. Previously the same situation would happen but the
 loop that would resize the array incrementally probably was not
 introspected properly and thus did not produce a warning.
 
 So there are basically three options:
 1) Silence the coverity warning

So on line just prior to CPU_ISSET_S either:

sa_assert(sizeof(unsigned long int) == sizeof(cpu_set_t));

or

/* coverity[overrun-local] */

Silences the warning

ACK for the current adjustment - your call if you want the sa_assert or
the coverity noise.

John

 2) File a bug against libc or something to fix the macro
 3) Reimplement CPU_ISSET_S internally. (Which I don't think will be
 possible since cpu_set_t does not look public)
 
 At any rate, there is no problem in libvirt's usage as it conforms to
 the example in the man page.
 
 As of this particular patch. It does not fix the coverity warning, but I
 think the intention and code flow is more apparent once it's applied.
 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-10 Thread Eric Blake 
On 06/10/2015 09:27 AM, John Ferlan wrote:

 So there are basically three options:
 1) Silence the coverity warning
 
 So on line just prior to CPU_ISSET_S either:
 
 sa_assert(sizeof(unsigned long int) == sizeof(cpu_set_t));

Is that true even on 32-bit platforms?

 
 or
 
 /* coverity[overrun-local] */

Might be safer, even though I generally prefer sa_assert() over /* magic
comment */

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-04 Thread Peter Krempa 
On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:
 
 
 On 06/04/2015 08:15 AM, Peter Krempa wrote:
  Refactor the code flow a bit more to clear coverity errors.
  
  Store the cpu count in an intermediate variable and reuse it rather than
  caluclating the index.
  ---
   src/util/virprocess.c | 11 +++
   1 file changed, 7 insertions(+), 4 deletions(-)
  
  diff --git a/src/util/virprocess.c b/src/util/virprocess.c
  index a38cb75..c07e5cd 100644
  --- a/src/util/virprocess.c
  +++ b/src/util/virprocess.c
  @@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
   size_t i;
   cpu_set_t *mask;
   size_t masklen;
  +size_t ncpus;
   virBitmapPtr ret = NULL;
  
   # ifdef CPU_ALLOC
   /* 262144 cpus ought to be enough for anyone */
  -masklen = CPU_ALLOC_SIZE(1024  8);
  -mask = CPU_ALLOC(1024  8);
  +ncpus = 1024  8;
  +masklen = CPU_ALLOC_SIZE(ncpus);
  +mask = CPU_ALLOC(ncpus);
  
   if (!mask) {
   virReportOOMError();
  @@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)
  
   CPU_ZERO_S(masklen, mask);
   # else
  +ncpus = 1024;
   if (VIR_ALLOC(mask)  0)
   return NULL;
  
  @@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
   goto cleanup;
   }
  
  -if (!(ret = virBitmapNew(masklen * 8)))
  +if (!(ret = virBitmapNew(ncpus)))
 goto cleanup;
  
  -for (i = 0; i  masklen * 8; i++) {
  +for (i = 0; i  ncpus; i++) {
   # ifdef CPU_ALLOC
   if (CPU_ISSET_S(i, masklen, mask))
 
 ^^^ Coverity still complains here
 
 No real change since previous...

Would you mind sharing the error after applying this patch?

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-04 Thread John Ferlan 


On 06/04/2015 11:13 AM, Peter Krempa wrote:
 On Thu, Jun 04, 2015 at 08:34:00 -0400, John Ferlan wrote:


 On 06/04/2015 08:15 AM, Peter Krempa wrote:
 Refactor the code flow a bit more to clear coverity errors.

 Store the cpu count in an intermediate variable and reuse it rather than
 caluclating the index.
 ---
  src/util/virprocess.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

 diff --git a/src/util/virprocess.c b/src/util/virprocess.c
 index a38cb75..c07e5cd 100644
 --- a/src/util/virprocess.c
 +++ b/src/util/virprocess.c
 @@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
  size_t i;
  cpu_set_t *mask;
  size_t masklen;
 +size_t ncpus;
  virBitmapPtr ret = NULL;

  # ifdef CPU_ALLOC
  /* 262144 cpus ought to be enough for anyone */
 -masklen = CPU_ALLOC_SIZE(1024  8);
 -mask = CPU_ALLOC(1024  8);
 +ncpus = 1024  8;
 +masklen = CPU_ALLOC_SIZE(ncpus);
 +mask = CPU_ALLOC(ncpus);

  if (!mask) {
  virReportOOMError();
 @@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)

  CPU_ZERO_S(masklen, mask);
  # else
 +ncpus = 1024;
  if (VIR_ALLOC(mask)  0)
  return NULL;

 @@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
  goto cleanup;
  }

 -if (!(ret = virBitmapNew(masklen * 8)))
 +if (!(ret = virBitmapNew(ncpus)))
goto cleanup;

 -for (i = 0; i  masklen * 8; i++) {
 +for (i = 0; i  ncpus; i++) {
  # ifdef CPU_ALLOC
  if (CPU_ISSET_S(i, masklen, mask))

 ^^^ Coverity still complains here

 No real change since previous...
 
 Would you mind sharing the error after applying this patch?
 
 Peter
 

Sure (it's cut-n-paste)

471 virBitmapPtr
472 virProcessGetAffinity(pid_t pid)
473 {
474 size_t i;
475 cpu_set_t *mask;
476 size_t masklen;
477 size_t ncpus;
478 virBitmapPtr ret = NULL;
479 
480 # ifdef CPU_ALLOC
481 /* 262144 cpus ought to be enough for anyone */

(1) Event assignment:   Assigning: ncpus = 262144UL.
Also see events:[cond_at_most][assignment][overrun-local]

482 ncpus = 1024  8;
483 masklen = CPU_ALLOC_SIZE(ncpus);
484 mask = CPU_ALLOC(ncpus);
485 

(2) Event cond_false:   Condition !mask, taking false branch

486 if (!mask) {
487 virReportOOMError();
488 return NULL;

(3) Event if_end:   End of if statement

489 }
490 
491 CPU_ZERO_S(masklen, mask);
492 # else
493 ncpus = 1024;
494 if (VIR_ALLOC(mask)  0)
495 return NULL;
496 
497 masklen = sizeof(*mask);
498 CPU_ZERO(mask);
499 # endif
500 

(4) Event cond_false:   Condition sched_getaffinity(pid, masklen, mask)  0, 
taking false branch

501 if (sched_getaffinity(pid, masklen, mask)  0) {
502 virReportSystemError(errno,
503  _(cannot get CPU affinity of process 
%d), pid);
504 goto cleanup;

(5) Event if_end:   End of if statement

505 }
506 

(6) Event cond_false:   Condition !(ret = virBitmapNew(ncpus)), taking false 
branch

507 if (!(ret = virBitmapNew(ncpus)))

(7) Event if_end:   End of if statement

508   goto cleanup;
509 

(8) Event cond_true:Condition i  ncpus, taking true branch
(13) Event loop_begin:  Jumped back to beginning of loop
(14) Event cond_true:   Condition i  ncpus, taking true branch
(15) Event cond_at_most:Checking i  ncpus implies that i may be up 
to 262143 on the true branch.
Also see events:[assignment][assignment][overrun-local]

510 for (i = 0; i  ncpus; i++) {
511 # ifdef CPU_ALLOC

(9) Event cond_true:Condition __cpu / 8  masklen, taking true branch
(10) Event cond_true:   Condition ((__cpu_mask const *)mask-__bits[__cpu / 
(64UL /* 8 * sizeof (__cpu_mask) */)]  (1UL /* (__cpu_mask)1 */  __cpu % 
(64UL /* 8 * sizeof (__cpu_mask) */))) != 0, taking true branch
(11) Event cond_true:   Condition ({...}), taking true branch
(16) Event assignment:  Assigning: __cpu = i. The value of __cpu may now 
be up to 262143.
(17) Event cond_true:   Condition __cpu / 8  masklen, taking true branch
(18) Event overrun-local:   Overrunning array of 16 8-byte elements at 
element index 4095 (byte offset 32760) by dereferencing pointer (__cpu_mask 
const *)mask-__bits + __cpu / 64UL.
Also see events:[assignment][cond_at_most]

512 if (CPU_ISSET_S(i, masklen, mask))
513 ignore_value(virBitmapSetBit(ret, i));
514 # else
515 if (CPU_ISSET(i, mask))
516 ignore_value(virBitmapSetBit(ret, i));
517 # endif

(12) Event loop:Jumping back to the beginning of the loop

518 }
519 
520  cleanup:
521 # ifdef CPU_ALLOC
522 CPU_FREE(mask);
523 # else
524 VIR_FREE(mask);
525 # endif
526 
527 return

[libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-04 Thread Peter Krempa 
Refactor the code flow a bit more to clear coverity errors.

Store the cpu count in an intermediate variable and reuse it rather than
caluclating the index.
---
 src/util/virprocess.c | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index a38cb75..c07e5cd 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
 size_t i;
 cpu_set_t *mask;
 size_t masklen;
+size_t ncpus;
 virBitmapPtr ret = NULL;

 # ifdef CPU_ALLOC
 /* 262144 cpus ought to be enough for anyone */
-masklen = CPU_ALLOC_SIZE(1024  8);
-mask = CPU_ALLOC(1024  8);
+ncpus = 1024  8;
+masklen = CPU_ALLOC_SIZE(ncpus);
+mask = CPU_ALLOC(ncpus);

 if (!mask) {
 virReportOOMError();
@@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)

 CPU_ZERO_S(masklen, mask);
 # else
+ncpus = 1024;
 if (VIR_ALLOC(mask)  0)
 return NULL;

@@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
 goto cleanup;
 }

-if (!(ret = virBitmapNew(masklen * 8)))
+if (!(ret = virBitmapNew(ncpus)))
   goto cleanup;

-for (i = 0; i  masklen * 8; i++) {
+for (i = 0; i  ncpus; i++) {
 # ifdef CPU_ALLOC
 if (CPU_ISSET_S(i, masklen, mask))
 ignore_value(virBitmapSetBit(ret, i));
-- 
2.4.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: Fix coverity warings in virProcessGetAffinity

2015-06-04 Thread John Ferlan 


On 06/04/2015 08:15 AM, Peter Krempa wrote:
 Refactor the code flow a bit more to clear coverity errors.
 
 Store the cpu count in an intermediate variable and reuse it rather than
 caluclating the index.
 ---
  src/util/virprocess.c | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)
 
 diff --git a/src/util/virprocess.c b/src/util/virprocess.c
 index a38cb75..c07e5cd 100644
 --- a/src/util/virprocess.c
 +++ b/src/util/virprocess.c
 @@ -474,12 +474,14 @@ virProcessGetAffinity(pid_t pid)
  size_t i;
  cpu_set_t *mask;
  size_t masklen;
 +size_t ncpus;
  virBitmapPtr ret = NULL;
 
  # ifdef CPU_ALLOC
  /* 262144 cpus ought to be enough for anyone */
 -masklen = CPU_ALLOC_SIZE(1024  8);
 -mask = CPU_ALLOC(1024  8);
 +ncpus = 1024  8;
 +masklen = CPU_ALLOC_SIZE(ncpus);
 +mask = CPU_ALLOC(ncpus);
 
  if (!mask) {
  virReportOOMError();
 @@ -488,6 +490,7 @@ virProcessGetAffinity(pid_t pid)
 
  CPU_ZERO_S(masklen, mask);
  # else
 +ncpus = 1024;
  if (VIR_ALLOC(mask)  0)
  return NULL;
 
 @@ -501,10 +504,10 @@ virProcessGetAffinity(pid_t pid)
  goto cleanup;
  }
 
 -if (!(ret = virBitmapNew(masklen * 8)))
 +if (!(ret = virBitmapNew(ncpus)))
goto cleanup;
 
 -for (i = 0; i  masklen * 8; i++) {
 +for (i = 0; i  ncpus; i++) {
  # ifdef CPU_ALLOC
  if (CPU_ISSET_S(i, masklen, mask))

^^^ Coverity still complains here

No real change since previous...

John
  ignore_value(virBitmapSetBit(ret, i));
 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list