Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6

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

On 18/02/16 14:16, Razvan Cojocaru wrote:
> On 02/18/2016 04:10 PM, Tamas K Lengyel wrote:
>> On Feb 18, 2016 03:46, "Razvan Cojocaru" <rcojocaru@bitdefender.com
>> <mailto:rcojocaru@bitdefender.com>> wrote:
>>> On 02/18/2016 12:45 PM, Corneliu ZUZU wrote:
>>>> c/s f5365e6: "xen/vm-events: Move parts of monitor_domctl code to
>> common-side",
>>>> introduced a use without initialization issue.
>>>> hvm_event_breakpoint calls hvm_event_traps(&req) and if sync is true
>> that
>>>> ors some bits into req->flags which was never initialised.
>>>> Reported by Coverity Scan.
>>>>
>>>> Initializes req @ hvm_event_breakpoint entry.
>>>>
>>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com
>> <mailto:czuzu@bitdefender.com>>
>>>> ---
>>>>  xen/arch/x86/hvm/event.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
>>>> index 874a36c..cb9c152 100644
>>>> --- a/xen/arch/x86/hvm/event.c
>>>> +++ b/xen/arch/x86/hvm/event.c
>>>> @@ -173,7 +173,7 @@ int hvm_event_breakpoint(unsigned long rip,
>>>>  {
>>>>      struct vcpu *curr = current;
>>>>      struct arch_domain *ad = &curr->domain->arch;
>>>> -    vm_event_request_t req;
>>>> +    vm_event_request_t req = {};
>> Should this be = { 0 } instead? Also, as I recall the request is not
>> initialized on any of the paths, so we might as well do it for all of
>> them, not just here. It would help avoid the listener erronously using
>> some fields that were not actually initialized as well.
> That should achieve the same thing. I first looked up "= {}" in the Xen
> source code and found:
>
> $ grep -R "= {}"
> arch/arm/domain_build.c: struct kernel_info kinfo = {};
> arch/x86/time.c: struct vcpu_time_info *u, _u = {};
> arch/x86/tboot.c: uint8_t nonce[16] = {};
> arch/x86/tboot.c: uint8_t nonce[16] = {};
> arch/x86/tboot.c: uint8_t nonce[16] = {};
> arch/x86/traps.c: unsigned char insns_before[8] = {}, insns_after[16] = {};
> arch/x86/x86_emulate/x86_emulate.c: union vex vex = {};
> arch/x86/x86_emulate/x86_emulate.c: struct x86_emulate_stub stub = {};
> arch/x86/cpu/mtrr/generic.c:struct mtrr_state mtrr_state = {};
> arch/x86/cpu/common.c:const struct cpu_dev *__read_mostly
> cpu_devs[X86_VENDOR_NUM] = {};
> arch/x86/domain.c: unsigned long total[MASK_EXTR(PGT_type_mask,
> PGT_type_mask) + 1] = {};
> tools/kconfig/expr.c: union string_value lval = {}, rval = {};
> common/grant_table.c: struct gnttab_copy_buf src = {};
> common/grant_table.c: struct gnttab_copy_buf dest = {};
>
> So I thought that that's the prevailing convention. I've now searched
> for "= {0}", and I see that that has also been done, so I suppose either
> way is fine as far as the coding style goes?
>
> $ grep -R "= {0}"
> arch/arm/mm.c:    lpae_t pte = {0};
> arch/arm/mm.c:    lpae_t pte = {0};
> drivers/char/exynos4210-uart.c:} exynos4210_com = {0};
> drivers/char/pl011.c:} pl011_com = {0};
> drivers/char/omap-uart.c:} omap_com = {0};
> drivers/char/scif-uart.c:} scif_com = {0};
> drivers/char/cadence-uart.c:} cuart_com = {0};
> tools/kconfig/nconf.gui.c:attributes_t attributes[ATTR_MAX+1] = {0};
> crypto/vmac.c:    uint64_t in[2] = {0}, out[2];

The two are equivalent as far as C goes.  I prefer the former as it is
slightly shorter.

~Andrew