| Message ID | 1455792328-28855-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > 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 = {}; > > switch ( type ) > { Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
On 18/02/16 10:45, 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. Coverity-ID: 1353192 > Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.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 = {}; > > switch ( type ) > {
On Feb 18, 2016 03:46, "Razvan Cojocaru" <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> > > --- > > 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. Tamas
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
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
On Thu, Feb 18, 2016 at 7:18 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > 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. OK, as long as they are equivalent I'm fine with either. I always use the {0} syntax, it looks more explicit to me, so just wanted to double-check that it means the same thing. Thanks, Tamas
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 = {}; switch ( type ) {
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> --- xen/arch/x86/hvm/event.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)