diff mbox

libxc: fix uninitialised usage of rc in meminit_hvm

Message ID 1454412800-2943-1-git-send-email-roger.pau@citrix.com
State New, archived
Headers show

Commit Message

Roger Pau Monné Feb. 2, 2016, 11:33 a.m. UTC
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é <roger.pau@citrix.com>
Reported-by: Olaf Hering <olaf@aepfle.de>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Wei Liu Feb. 2, 2016, 12:37 p.m. UTC | #1
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é <roger.pau@citrix.com>
> Reported-by: Olaf Hering <olaf@aepfle.de>

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

> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index ef474a8..08337b2 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -1259,7 +1259,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
>      unsigned long p2m_size;
>      unsigned long target_pages = dom->target_pages;
>      unsigned long cur_pages, cur_pfn;
> -    int rc;
> +    int rc = 0;
>      xen_capabilities_info_t caps;
>      unsigned long stat_normal_pages = 0, stat_2mb_pages = 0, 
>          stat_1gb_pages = 0;
> -- 
> 2.5.4 (Apple Git-61)
>
Ian Campbell Feb. 3, 2016, 10:30 a.m. UTC | #2
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é <roger.pau@citrix.com>
> > Reported-by: Olaf Hering <olaf@aepfle.de>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

This is CID 1351229, I think?

** 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?

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?

Ian.
Wei Liu Feb. 3, 2016, 10:42 a.m. UTC | #3
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é <roger.pau@citrix.com>
> > > Reported-by: Olaf Hering <olaf@aepfle.de>
> > 
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> This is CID 1351229, I think?
> 

Yes, I think so.

> ** 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?
> 

No, there is no if ( rc != 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 = 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?
> 

Yeah, that should ideally be fixed, too.

Wei.

> Ian.
Roger Pau Monné Feb. 3, 2016, 10:44 a.m. UTC | #4
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@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é <roger.pau@citrix.com>
>>> Reported-by: Olaf Hering <olaf@aepfle.de>
>>
>> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> 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.

> 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.

Roger.
Ian Campbell Feb. 3, 2016, 10:54 a.m. UTC | #5
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@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é <roger.pau@citrix.com>
> > > > Reported-by: Olaf Hering <olaf@aepfle.de>
> > > 
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > 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.
diff mbox

Patch

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index ef474a8..08337b2 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1259,7 +1259,7 @@  static int meminit_hvm(struct xc_dom_image *dom)
     unsigned long p2m_size;
     unsigned long target_pages = dom->target_pages;
     unsigned long cur_pages, cur_pfn;
-    int rc;
+    int rc = 0;
     xen_capabilities_info_t caps;
     unsigned long stat_normal_pages = 0, stat_2mb_pages = 0, 
         stat_1gb_pages = 0;