From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wei Liu <wei.liu2@citrix.com>
Subject: Re: [PATCH] libxc: fix uninitialised usage of rc in
	meminit_hvm
Date: Wed, 3 Feb 2016 10:42:51 +0000
Message-ID: <20160203104251.GF23178@citrix.com>
References: <20160201071735.GA21091@gmail.com>
	<1454412800-2943-1-git-send-email-roger.pau@citrix.com>
	<20160202123721.GA25660@citrix.com>
	<1454495454.25207.47.camel@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=834ba7b9c=wei.liu2@citrix.com>)
	id 1aQutj-00066S-Kk
	for xen-devel@lists.xenproject.org; Wed, 03 Feb 2016 10:42:55 +0000
Content-Disposition: inline
In-Reply-To: <1454495454.25207.47.camel@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Campbell <ian.campbell@citrix.com>
Cc: xen-devel@lists.xenproject.org, Roger Pau Monne <royger@FreeBSD.org>, Wei Liu <wei.liu2@citrix.com>, Ian Jackson <ian.jackson@eu.citrix.com>, Roger Pau Monne <roger.pau@citrix.com>
List-Id: xen-devel@lists.xenproject.org

On Wed, Feb 03, 2016 at 10:30:54AM +0000, Ian Campbell wrote:
> 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@FreeBSD.org>
> > > =

> > > 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=E9 <roger.pau@citrix.com>
> > > Reported-by: Olaf Hering <olaf@aepfle.de>
> > =

> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> =

> This is CID=A01351229, I think?
> =


Yes, I think so.

> ** CID 1351229:=A0=A0Uninitialized variables=A0=A0(UNINIT)
> > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> >=A0
> >=A0
> > _______________________________________________________________________=
_________________________________
> > *** CID 1351229:=A0=A0Uninitialized variables=A0=A0(UNINIT)
> > /tools/libxc/xc_dom_x86.c: 1443 in meminit_hvm()
> > 1437=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0cur_pages =3D 0x=
c0;
> > 1438=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0stat_normal_page=
s +=3D 0xc0;
> > 1439=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0}
> > 1440=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0else
> > 1441=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0cur_pages =3D vm=
emranges[vmemid].start >> PAGE_SHIFT;
> > 1442=A0=A0=A0=A0=A0
> > >>>=A0=A0=A0=A0=A0CID 1351229:=A0=A0Uninitialized variables=A0=A0(UNINI=
T)
> > >>>=A0=A0=A0=A0=A0Using uninitialized value "rc".
> > 1443=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0while ( (rc =3D=3D 0) && (en=
d_pages > cur_pages) )
> > 1444=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0{
> > 1445=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* Clip count to=
 maximum 1GB extent. */
> > 1446=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0unsigned long co=
unt =3D end_pages - cur_pages;
> > 1447=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0unsigned long ma=
x_pages =3D SUPERPAGE_1GB_NR_PFNS;
> > 1448 =A0 =A0
> =

> Note that this while loop ends with:
> =A0=A0=A0=A0=A0=A0=A0=A0if ( rc !=3D 0 )
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0break;
> and there are no continue statements.
> =

> Therefore I wonder if we would be better off removing the rc =3D=3D 0 par=
t of
> the loop condition?
> =


No, there is no if ( rc !=3D 0 )  inside the said while loop.  That "if"
is for the outer for loop.

We could add a test for the while loop, if that looks clearer to you.

> 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 =3D xc_domain_populate_physmap_exac=
t"
> becoming conditional on=A0device_model. What is also concerning is the la=
ck
> of error checking on that call -- is it really ok to just barrel on under
> these circumstance?
> =


Yeah, that should ideally be fixed, too.

Wei.

> Ian.