| Message ID | 1454412800-2943-1-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) >
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.
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.
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.
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 --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;