From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH 1/1] hvm.c: Prevent gcc uninitialised var warning Date: Wed, 25 Mar 2015 16:03:54 -0400 Message-ID: <551314AA.3020204@terremark.com> References: <1427119288-19348-1-git-send-email-dslutz@verizon.com> <5512CE05.6010105@citrix.com> <5512E6CC020000780006D883@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5512E6CC020000780006D883@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Andrew Cooper Cc: Ian Murray , Keir Fraser , Ian Campbell , Don Slutz , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 03/25/15 11:48, Jan Beulich wrote: >>>> On 25.03.15 at 16:02, wrote: >> On 23/03/15 15:01, Don Slutz wrote: >>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3 reports: >>> ---------------------------------------------------------------------- >>> hvm.c: In function `hvm_create_ioreq_server': >>> hvm.c:487:18: error: `bufioreq_pfn' may be used uninitialised in this >> function >>> [-Werror=uninitialized] >>> hvm.c:718:30: note: `bufioreq_pfn' was declared here >>> ---------------------------------------------------------------------- >>> >>> My code analysis says that gcc is wrong, but initilize the variable >>> to prevent the gcc warning. >>> >>> Reported-by: Ian Murray >>> Signed-off-by: Don Slutz >> >> GCC is correct and there is path through this function where >> bufioreq_pfn is used while uninitialised (non-debug build, is_default = >> 1, handle_bufioreq = 0). > Looks like you are talking about the ASSERT(handle_bufioreq). But that does not leave bufioreq_pfn uninitialised. Currently you cannot get here in that state. The only way to have is_default=1 is: rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL); Which should prevent "handle_bufioreq == 0" > I'm not seeing it: When is_default is set, bufioreq_pfn gets > initialized in the first if()'s body. > >> As an aside, the compiler is in a very easy position to spot this. The >> error means that GCC has positively identified a basic block which does >> use bufioreq_pfn before it has been initialised. >> If the compiler is right, how come the messages says: may be used which to me says that it's determination is not 100% >> I observe that the patch has been committed, but it merely hides the >> problem and doesn't fix it; hvm_free_ioreq_gmfn(d, bufioreq_pfn) will >> still clobber arbitrary memory. > hvm_free_ioreq_gmfn() does have issues. And since it is possible to call it with d->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_PFN] which can be anything. > I committed the patch based on me not being able to find a path > where the variable would actually be used uninitialized (and it is > known that various gcc versions have varying levels of problems > with correctly identifying such issues). If indeed there is a path > that I'm not seeing, then I'd indeed favor reverting and putting > in a proper, backportable fix. > I still cannot find a path where this fails. However I can provide a patch that does 2 things: 1) Change the "ASSERT(handle_bufioreq)" to "BUG_ON(handle_bufioreq);" (not sure it is required). 2) Add checking to hvm_free_ioreq_gmfn() to prevent memory clobber since it's arg may come from an HVM_PARAM. -Don Slutz > Jan >