From mboxrd@z Thu Jan 1 00:00:00 1970 From: Razvan Cojocaru Subject: Re: [PATCH] x86/hvm_event: fix uninitialized struct field usage introduced by c/s f5365e6 Date: Thu, 18 Feb 2016 16:16:36 +0200 Message-ID: <56C5D244.4050602@bitdefender.com> References: <1455792328-28855-1-git-send-email-czuzu@bitdefender.com> <56C5A139.1050208@bitdefender.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: Andrew Cooper , xen-devel@lists.xen.org, Keir Fraser , Jan Beulich , Corneliu ZUZU List-Id: xen-devel@lists.xenproject.org On 02/18/2016 04:10 PM, Tamas K Lengyel wrote: > > On Feb 18, 2016 03:46, "Razvan Cojocaru" > 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 > >> > --- >> > 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