Re: [Xen-devel] [PATCH] Initialize ioreq_t in hvmemul_do_io()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.05.14 at 15:47, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/05/14 14:27, Jan Beulich wrote:
>>>>> On 22.05.14 at 15:20, <Paul.Durrant@xxxxxxxxxx> wrote:
>>>>  -----Original Message-----
>>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>> Sent: 22 May 2014 14:14
>>>> To: Andrew Cooper; Paul Durrant
>>>> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
>>>> Subject: Re: [PATCH] Initialize ioreq_t in hvmemul_do_io()
>>>>
>>>>>>> On 22.05.14 at 14:48, <paul.durrant@xxxxxxxxxx> wrote:
>>>>> All relevant fields of the ioreq_t are properly initialized but coverity
>>>>> complains about vp_eport being uninitialized (ID 1215178) because it is
>>>>> the subject of a struct copy in hvm_send_assist_req().
>>>>>
>>>>> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
>>>>> Cc: Keir Fraser <keir@xxxxxxx>
>>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>> ---
>>>>>  xen/arch/x86/hvm/emulate.c |    2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>>>>> index 904c71a..e516194 100644
>>>>> --- a/xen/arch/x86/hvm/emulate.c
>>>>> +++ b/xen/arch/x86/hvm/emulate.c
>>>>> @@ -57,7 +57,7 @@ static int hvmemul_do_io(
>>>>>      int value_is_ptr = (p_data == NULL);
>>>>>      struct vcpu *curr = current;
>>>>>      struct hvm_vcpu_io *vio;
>>>>> -    ioreq_t p;
>>>>> +    ioreq_t p = { 0 };
>>>> To be honest, I dislike fixes like this to address tool shortcomings. It's
>>>> not like this is giaramteed to be just a single instruction that gets
>>>> added (maybe the compiler can figure this, but I would want to rely
>>>> on it.
>>>>
>>> I could always change hvm_send_assist_req() to only copy the relevant 
>>> fields 
>>> from the proto structure, as it used to in earlier versions of my patch.
>> But the structure copy is certainly more efficient than the piece-wise
>> copying.
> 
> When interpreting the coverity results, I accidentally missed the second
> occurrence of this issue.
> 
> It is possible to "goto finished_access" and have p.addr and p.dir
> uninitialised when tracing the IO.
> 
> By far the safest option is just initialise the entire structure to 0.

Then the better choice imo is to move as much as possible of the
initialization into the initializer. That might even allow cleaning up
variables - for example value_is_ptr could clearly be replaced by
p.data_is_ptr - and it would reduce the life range of e.g. df.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel