Re: [Xen-devel] [PATCH] libxc: fix uninitialised usage of rc in meminit_hvm

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2016-02-03 at 11:44 +0100, Roger Pau Monnà wrote:
> El 3/2/16 a les 11:30, Ian Campbell ha escrit:
> > On Tue, 2016-02-02 at 12:37 +0000, Wei Liu wrote:
> > > On Tue, Feb 02, 2016 at 12:33:20PM +0100, Roger Pau Monne wrote:
> > > > From: Roger Pau Monne <royger@xxxxxxxxxxx>
> > > > 
> > > > Due to the HVMlite changes there's a chance that the value in rc is
> > > > checked
> > > > without being initialised. Fix this by initialising it to 0.
> > > > 
> > > > Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> > > > Reported-by: Olaf Hering <olaf@xxxxxxxxx>
> > > 
> > > Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > 
> > This is CID 1351229, I think?
> 
> Looks like, according the the description below.
> 
> > 
> > ** CID 1351229:ÂÂUninitialized variablesÂÂ(UNINIT)
> > > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > > Â
> > > Â
> > > _____________________________________________________________________
> > > ___________________________________
> > > *** CID 1351229:ÂÂUninitialized variablesÂÂ(UNINIT)
> > > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > > 1437ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcur_pages = 0xc0;
> > > 1438ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstat_normal_pages += 0xc0;
> > > 1439ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > > 1440ÂÂÂÂÂÂÂÂÂÂÂÂÂelse
> > > 1441ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcur_pages = vmemranges[vmemid].start >>
> > > PAGE_SHIFT;
> > > 1442ÂÂÂÂÂ
> > > > > > ÂÂÂÂÂCID 1351229:ÂÂUninitialized variablesÂÂ(UNINIT)
> > > > > > ÂÂÂÂÂUsing uninitialized value "rc".
> > > 1443ÂÂÂÂÂÂÂÂÂÂÂÂÂwhile ( (rc == 0) && (end_pages > cur_pages) )
> > > 1444ÂÂÂÂÂÂÂÂÂÂÂÂÂ{
> > > 1445ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* Clip count to maximum 1GB extent. */
> > > 1446ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned long count = end_pages - cur_pages;
> > > 1447ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned long max_pages = SUPERPAGE_1GB_NR_PFNS;
> > > 1448ÂÂÂÂ
> > 
> > Note that this while loop ends with:
> > ÂÂÂÂÂÂÂÂif ( rc != 0 )
> > ÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > and there are no continue statements.
> > 
> > Therefore I wonder if we would be better off removing the rc == 0 part
> > of
> > the loop condition?
> 
> We could, but I think we would still have the same issue with the "if (
> rc != 0 )" at the end of the loop, AFAICT rc is never unconditionally
> set inside of the loop itself, so gcc and coverity would still complain
> about uninitialized usage.

Right, I was looking at the wrong loop as Wei pointed out.

I think "rc = 0" before the while might be a reasonable option.

> > The issue with this patch is the usual one that it will hide other
> > unintentional uses of rc before it is set to a good value.
> > 
> > This issue was exposed by a prior "rc =
> > xc_domain_populate_physmap_exact"
> > becoming conditional on device_model. What is also concerning is the
> > lack
> > of error checking on that call -- is it really ok to just barrel on
> > under
> > these circumstance?
> 
> Hm, I guess we then rely on the rc == 0 at the start of the while loop
> in order to bail out. IMHO the logic in this function is overly
> complicated.

Indeed, although we do some other (I suppose pointless) work first in that
case too.

Moving some of it into separate helpers would be a nice further cleanup.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel