Re: Coverity tidying

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/12/15 05:16, Joshua Otto wrote:
> On Mon, Dec 14, 2015 at 11:08:43AM +0000, Ian Campbell wrote:
>> On Sat, 2015-12-12 at 17:07 -0500, Joshua Otto wrote:
>>> On Fri, Dec 11, 2015 at 01:52:41PM +0000, Ian Campbell wrote:
>>>> Cool! Just to be clear, you are looking for one project for the 3 of
>>>> you to
>>>> work on as a group (vs 3 individual projects), is that right?
>>> Yes, that's right.
>>>
>>>> It's been a while since there has been a scan run, I did one yesterday but
>>>> it is taking an unusually long time to get the results back. Hopefully
>>>> we'll have an up to date set of defects early next week and I can have a
>>>> scrobble around for some interesting ones for you guys to take a look at.
>>> That would be perfect, thanks!
>> Results are in. I've cherry-picked a few of the new issues below. I've not
>> checked carefully for false +ves.
>>
>> Not a great deal of massive thrills in there, but some one liners etc to
>> dip your toes in I guess.
> These patches address the Coverity scan issues identified below that appear to
> be actual problems.  For issues that we believe to be false positives, we
> briefly explain why.
>
> We've attempted to CC maintainers according to get_maintainer.pl.
>
>> ________________________________________________________________________________________________________
>> *** CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
>> /xen/arch/x86/hvm/svm/intr.c: 95 in svm_enable_intr_window()
>> 89                 struct vmcb_struct *gvmcb = nv->nv_vvmcx;
>> 90
>> 91                 /* check if l1 guest injects interrupt into l2 guest via vintr.
>> 92                  * return here or l2 guest looses interrupts, otherwise.
>> 93                  */
>> 94                 ASSERT(gvmcb != NULL);
>>>>>       CID 1343310:  Code maintainability issues  (UNUSED_VALUE)
>>>>>       Assigning value from "vmcb_get_vintr(gvmcb)" to "intr" here, but that stored value is overwritten before it can be used.
>> 95                 intr = vmcb_get_vintr(gvmcb);
>> 96                 if ( intr.fields.irq )
>> 97                     return;
>> 98             }
>> 99         }
>> 100
> intr is used on the next line, so this appears to be a false positive without an
> obvious rephrasing that Coverity would accept.

The error message isn't fantastic, but the complaint that Coverity has 
is that we store intr here, then unilaterally store it again slightly 
lower in the function, no matter what value it had (with the early 
return presumably not being taken into account).

The error would probably be resolved if lines 95 and 96 turned into "if 
( vmcb_get_vintr(gvmcb).fields.irq )"

>
>> ________________________________________________________________________________________________________
>> *** CID 1343309:  Control flow issues  (UNREACHABLE)
>> /tools/libxl/libxl.c: 5575 in libxl_get_scheduler()
>> 5569     {
>> 5570         libxl_scheduler sched, ret;
>> 5571         GC_INIT(ctx);
>> 5572         if ((ret = xc_sched_id(ctx->xch, (int *)&sched)) != 0) {
>> 5573             LOGE(ERROR, "getting domain info list");
>> 5574             return ERROR_FAIL;
>>>>>       CID 1343309:  Control flow issues  (UNREACHABLE)
>>>>>       This code cannot be reached: "libxl__free_all(gc);".
>> 5575             GC_FREE;
>> 5576         }
>> 5577         GC_FREE;
>> 5578         return sched;
>> 5579     }
>> 5580
>>
>> As well as putting GC_FREE in the right place this function could be
>> reworked to follow the recommendations in tools/libxl/CODING_STYLE.
> This issue is addressed by patches 1 and 2.
>
>> ** CID 1343307:    (RESOURCE_LEAK)
>> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
>> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
>> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>>
>>
>> ________________________________________________________________________________________________________
>> *** CID 1343307:    (RESOURCE_LEAK)
>> /tools/libxl/libxl_dm.c: 746 in libxl__dm_runas_helper()
>> 740             ret = getpwnam_r(username, &pwd, buf, buf_size, &user);
>> 741             if (ret == ERANGE) {
>> 742                 buf_size += 128;
>> 743                 continue;
>> 744             }
>> 745             if (ret != 0)
>>>>>       CID 1343307:    (RESOURCE_LEAK)
>>>>>       Variable "buf" going out of scope leaks the storage it points to.
>> 746                 return ERROR_FAIL;
>> 747             if (user != NULL)
>> 748                 return 1;
>> 749             return 0;
>> 750         }
>> 751     }
>> /tools/libxl/libxl_dm.c: 748 in libxl__dm_runas_helper()
>> 742                 buf_size += 128;
>> 743                 continue;
>> 744             }
>> 745             if (ret != 0)
>> 746                 return ERROR_FAIL;
>> 747             if (user != NULL)
>>>>>       CID 1343307:    (RESOURCE_LEAK)
>>>>>       Variable "buf" going out of scope leaks the storage it points to.
>> 748                 return 1;
>> 749             return 0;
>> 750         }
>> 751     }
>> 752
>> 753     static int libxl__build_device_model_args_new(libxl__gc *gc,
>> /tools/libxl/libxl_dm.c: 749 in libxl__dm_runas_helper()
>> 743                 continue;
>> 744             }
>> 745             if (ret != 0)
>> 746                 return ERROR_FAIL;
>> 747             if (user != NULL)
>> 748                 return 1;
>>>>>       CID 1343307:    (RESOURCE_LEAK)
>>>>>       Variable "buf" going out of scope leaks the storage it points to.
>> 749             return 0;
>> 750         }
>> 751     }
>> 752
>> 753     static int libxl__build_device_model_args_new(libxl__gc *gc,
>> 754                                             const char *dm, int guest_domid,
> This appears to be a false positive - libxl__realloc() ensures that any
> new allocations are added to the gc, and that subsequent reallocations
> bring the gc up to date, so exiting the function at any time should be
> safe.

Correct.  Coverity is unable to track object ownership information when 
you start playing containerof() games to put it in a list.

~Andrew