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

[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 ret;
528  	}