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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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];


Thanks,
Razvan