From: Alex Jia&lt;ajia(a)redhat.com&gt; If the function lxcSetupLoopDevices(def,&nloopDevs,&loopDevs) failed, the variable loopDevs will keep a initial NULL value, however, the function VIR_FORCE_CLOSE(loopDevs[i]) will directly deref it. * rc/lxc/lxc_controller.c: fixed a null pointer dereference. Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; --- src/lxc/lxc_controller.c | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c4e7832..024756d 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1017,8 +1017,11 @@ cleanup: VIR_FORCE_CLOSE(containerhandshake[0]); VIR_FORCE_CLOSE(containerhandshake[1]); - for (i = 0 ; i< nloopDevs ; i++) - VIR_FORCE_CLOSE(loopDevs[i]);

+ if (loopDevs) { + for (i = 0 ; i< nloopDevs ; i++) + VIR_FORCE_CLOSE(loopDevs[i]); + } + VIR_FREE(loopDevs); if (container> 1) {

On Thu, Oct 27, 2011 at 10:06:09AM +0200, Peter Krempa wrote: > On 10/27/2011 09:18 AM, ajia(a)redhat.com wrote: >> From: Alex Jia&lt;ajia(a)redhat.com&gt; >> > > Indeed, this situation might happen if memory reallocation fails > after some iterations of the loop inside of lxcSetupLoopDevices, > leaving nloopDevs assigned to some value, but loopDevs being NULL. No it can't. If VIR_REALLOC_N fails, it guarentees that the original pointer is left at its original value. It can't become NULL. This is critical, because we need to release any FDs that were stored into loopDevs in earlier iterations of the loop

>> + if (loopDevs) { >> + for (i = 0 ; i< nloopDevs ; i++) >> + VIR_FORCE_CLOSE(loopDevs[i]); > > > and pushed. This is actually *wrong*. You now leak any file descriptors that were stored in loopDevs prior to the VIR_REALLOC_N failure. Please revert this hunk

Daniel

On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia(a)redhat.com wrote: > From: Alex Jia&lt;ajia(a)redhat.com&gt; > > Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) > can make sure mounts is a none null value, so it is redundant to check if > mounts is null. VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, in theory there can be zero iterations of the loop, in which case 'mounts' will be NULL at the point qsort is called. > * src/lxc/lxc_container.c: remove redundant check. > > Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; > --- > src/lxc/lxc_container.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index 06ccf7e..f4879c3 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) > } > endmntent(procmnt); > > - if (mounts) > - qsort(mounts, nmounts, sizeof(mounts[0]), > - lxcContainerChildMountSort); > + qsort(mounts, nmounts, sizeof(mounts[0]), > + lxcContainerChildMountSort); > > for (i = 0 ; i< nmounts ; i++) { > VIR_DEBUG("Umount %s", mounts[i]); NACK, the check for NULL is correct Daniel

On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote: > On 10/27/2011 04:20 PM, Daniel P. Berrange wrote: >> On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia(a)redhat.com wrote: >>> From: Alex Jia&lt;ajia(a)redhat.com&gt; >>> >>> Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) >>> can make sure mounts is a none null value, so it is redundant to check if >>> mounts is null. >> VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, >> in theory there can be zero iterations of the loop, in which case >> 'mounts' will be NULL at the point qsort is called. >> >>> * src/lxc/lxc_container.c: remove redundant check. >>> >>> Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; >>> --- >>> src/lxc/lxc_container.c | 5 ++--- >>> 1 files changed, 2 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >>> index 06ccf7e..f4879c3 100644 >>> --- a/src/lxc/lxc_container.c >>> +++ b/src/lxc/lxc_container.c >>> @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) >>> } >>> endmntent(procmnt); >>> >>> - if (mounts) >>> - qsort(mounts, nmounts, sizeof(mounts[0]), >>> - lxcContainerChildMountSort); >>> + qsort(mounts, nmounts, sizeof(mounts[0]), >>> + lxcContainerChildMountSort); >>> >>> for (i = 0 ; i< nmounts ; i++) { >>> VIR_DEBUG("Umount %s", mounts[i]); >> NACK, the check for NULL is correct >> >> >> Daniel > Hi Daniel, > My comment is wrong, but the following judgement should make sure > mounts is non null: > <snip> > if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) {

> endmntent(procmnt); > virReportOOMError(); > return -1; > } > </snip> That is ensuring that the pointer in the array element value is non-NULL. The "if (mounts) qsort(...)" check is against the array itself, rather than the element. Regards, Daniel

On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote: > On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: >> On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote: >>> On 10/27/2011 04:20 PM, Daniel P. Berrange wrote: >>>> On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia(a)redhat.com wrote: >>>>> From: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>> >>>>> Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) >>>>> can make sure mounts is a none null value, so it is redundant to check if >>>>> mounts is null. >>>> VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, >>>> in theory there can be zero iterations of the loop, in which case >>>> 'mounts' will be NULL at the point qsort is called. >>>> >>>>> * src/lxc/lxc_container.c: remove redundant check. >>>>> >>>>> Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>> --- >>>>> src/lxc/lxc_container.c | 5 ++--- >>>>> 1 files changed, 2 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >>>>> index 06ccf7e..f4879c3 100644 >>>>> --- a/src/lxc/lxc_container.c >>>>> +++ b/src/lxc/lxc_container.c >>>>> @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) >>>>> } >>>>> endmntent(procmnt); >>>>> >>>>> - if (mounts) >>>>> - qsort(mounts, nmounts, sizeof(mounts[0]), >>>>> - lxcContainerChildMountSort); >>>>> + qsort(mounts, nmounts, sizeof(mounts[0]), >>>>> + lxcContainerChildMountSort); >>>>> >>>>> for (i = 0 ; i< nmounts ; i++) { >>>>> VIR_DEBUG("Umount %s", mounts[i]); >>>> NACK, the check for NULL is correct >>>> >>>> >>>> Daniel >>> Hi Daniel, >>> My comment is wrong, but the following judgement should make sure >>> mounts is non null: >>> <snip> >>> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { > If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, > whether we need to check 'mounts' to avoid dereferencing a NULL > pointer in here again, for instance: > > if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there is at least 1 iteration of the while() loop. The "if (mounts) qsort" code is *outside* the while() loop, and has to cope with the scenario where the while() loop had *zero* iterations.

Regards, Daniel

On 10/27/2011 05:59 PM, Daniel P. Berrange wrote: > On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote: >> On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: >>> On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote: >>>> On 10/27/2011 04:20 PM, Daniel P. Berrange wrote: >>>>> On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia(a)redhat.com wrote: >>>>>> From: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>>> >>>>>> Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) >>>>>> can make sure mounts is a none null value, so it is redundant to check if >>>>>> mounts is null. >>>>> VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, >>>>> in theory there can be zero iterations of the loop, in which case >>>>> 'mounts' will be NULL at the point qsort is called. >>>>> >>>>>> * src/lxc/lxc_container.c: remove redundant check. >>>>>> >>>>>> Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>>> --- >>>>>> src/lxc/lxc_container.c | 5 ++--- >>>>>> 1 files changed, 2 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >>>>>> index 06ccf7e..f4879c3 100644 >>>>>> --- a/src/lxc/lxc_container.c >>>>>> +++ b/src/lxc/lxc_container.c >>>>>> @@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) >>>>>> } >>>>>> endmntent(procmnt); >>>>>> >>>>>> - if (mounts) >>>>>> - qsort(mounts, nmounts, sizeof(mounts[0]), >>>>>> - lxcContainerChildMountSort); >>>>>> + qsort(mounts, nmounts, sizeof(mounts[0]), >>>>>> + lxcContainerChildMountSort); >>>>>> >>>>>> for (i = 0 ; i< nmounts ; i++) { >>>>>> VIR_DEBUG("Umount %s", mounts[i]); >>>>> NACK, the check for NULL is correct >>>>> >>>>> >>>>> Daniel >>>> Hi Daniel, >>>> My comment is wrong, but the following judgement should make sure >>>> mounts is non null: >>>> <snip> >>>> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { >> If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, >> whether we need to check 'mounts' to avoid dereferencing a NULL >> pointer in here again, for instance: >> >> if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { > The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there > is at least 1 iteration of the while() loop. The "if (mounts) qsort" > code is *outside* the while() loop, and has to cope with the scenario > where the while() loop had *zero* iterations. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-( 882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i < nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.*

On 10/27/2011 05:59 PM, Daniel P. Berrange wrote: >On Thu, Oct 27, 2011 at 05:54:20PM +0800, Alex Jia wrote: >>On 10/27/2011 05:26 PM, Daniel P. Berrange wrote: >>>On Thu, Oct 27, 2011 at 04:33:50PM +0800, Alex Jia wrote: >>>>On 10/27/2011 04:20 PM, Daniel P. Berrange wrote: >>>>>On Thu, Oct 27, 2011 at 03:18:01PM +0800, ajia(a)redhat.com wrote: >>>>>>From: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>>> >>>>>>Detected by cppcheck, the previous function VIR_REALLOC_N(mounts, nmounts+1) >>>>>>can make sure mounts is a none null value, so it is redundant to check if >>>>>>mounts is null. >>>>>VIR_REALLOC_N is only executed inside the loop. Although it is unlikely, >>>>>in theory there can be zero iterations of the loop, in which case >>>>>'mounts' will be NULL at the point qsort is called. >>>>> >>>>>>* src/lxc/lxc_container.c: remove redundant check. >>>>>> >>>>>>Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; >>>>>>--- >>>>>> src/lxc/lxc_container.c | 5 ++--- >>>>>> 1 files changed, 2 insertions(+), 3 deletions(-) >>>>>> >>>>>>diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c >>>>>>index 06ccf7e..f4879c3 100644 >>>>>>--- a/src/lxc/lxc_container.c >>>>>>+++ b/src/lxc/lxc_container.c >>>>>>@@ -879,9 +879,8 @@ static int lxcContainerUnmountOldFS(void) >>>>>> } >>>>>> endmntent(procmnt); >>>>>> >>>>>>- if (mounts) >>>>>>- qsort(mounts, nmounts, sizeof(mounts[0]), >>>>>>- lxcContainerChildMountSort); >>>>>>+ qsort(mounts, nmounts, sizeof(mounts[0]), >>>>>>+ lxcContainerChildMountSort); >>>>>> >>>>>> for (i = 0 ; i< nmounts ; i++) { >>>>>> VIR_DEBUG("Umount %s", mounts[i]); >>>>>NACK, the check for NULL is correct >>>>> >>>>> >>>>>Daniel >>>>Hi Daniel, >>>>My comment is wrong, but the following judgement should make sure >>>>mounts is non null: >>>><snip> >>>> if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) { >>If the previous VIR_REALLOC_N can't make sure 'mounts' is non null, >>whether we need to check 'mounts' to avoid dereferencing a NULL >>pointer in here again, for instance: >> >>if (!mounts || !(mounts[nmounts++] = strdup(mntent.mnt_dir))) { >The check for VIR_REALLOC_N ensures that mounts is non-NULL *if* there >is at least 1 iteration of the while() loop. The "if (mounts) qsort" >code is *outside* the while() loop, and has to cope with the scenario >where the while() loop had *zero* iterations. Hmm, I just saw codes again, it indeed is a issue, however, I fixed a error place :-( 882 if (mounts) 883 qsort(mounts, nmounts, sizeof(mounts[0]), 884 lxcContainerChildMountSort); 885 *Notes: if mounts is NULL, the above 'if' clause can't be executed, but the following codes will be run.* 886 for (i = 0 ; i < nmounts ; i++) { 887 VIR_DEBUG("Umount %s", mounts[i]); *Notes: dereference a NULL pointer in here.* 888 if (umount(mounts[i]) < 0) { Notes: same as above. 889 virReportSystemError(errno, 890 _("Failed to unmount '%s'"), 891 mounts[i]); Notes: same as above. 892 return -1; 893 } 894 VIR_FREE(mounts[i]); Notes: same as above. 895 } 896 VIR_FREE(mounts); 897 898 return 0; 899 } So we should use a '{}' to surround with 883: qsort...894: VIR_FREE(mounts[i]);

On Thu, Oct 27, 2011 at 03:17:59PM +0800, ajia(a)redhat.com wrote: > From: Alex Jia&lt;ajia(a)redhat.com&gt; > > Cppcheck detected a syntaxError on lxcDomainInterfaceStats. > > * src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats. > > Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; > --- > src/lxc/lxc_driver.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index f08e8d1..06bfa85 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -2671,6 +2671,7 @@ static int > lxcDomainInterfaceStats(virDomainPtr dom, > const char *path ATTRIBUTE_UNUSED, > struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) > +{ > lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); > return -1; > } Hmm, I dunno why we even have a '#ifdef __linux__' in that file since the entire file is Linux specific. We should just kill this #else clause entirely

Daniel

On Thu, Oct 27, 2011 at 04:51:05PM +0800, Alex Jia wrote: > On 10/27/2011 04:15 PM, Daniel P. Berrange wrote: >> On Thu, Oct 27, 2011 at 03:17:59PM +0800, ajia(a)redhat.com wrote: >>> From: Alex Jia&lt;ajia(a)redhat.com&gt; >>> >>> Cppcheck detected a syntaxError on lxcDomainInterfaceStats. >>> >>> * src/lxc/lxc_driver.c: fixed missing '{' in the function lxcDomainInterfaceStats. >>> >>> Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; >>> --- >>> src/lxc/lxc_driver.c | 1 + >>> 1 files changed, 1 insertions(+), 0 deletions(-) >>> >>> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c >>> index f08e8d1..06bfa85 100644 >>> --- a/src/lxc/lxc_driver.c >>> +++ b/src/lxc/lxc_driver.c >>> @@ -2671,6 +2671,7 @@ static int >>> lxcDomainInterfaceStats(virDomainPtr dom, >>> const char *path ATTRIBUTE_UNUSED, >>> struct _virDomainInterfaceStats *stats ATTRIBUTE_UNUSED) >>> +{ >>> lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); >>> return -1; >>> } >> Hmm, I dunno why we even have a '#ifdef __linux__' in that file >> since the entire file is Linux specific. We should just kill this >> #else clause entirely > You mean we should remove the following lines rather then reserving > the function as > a optional branch: > #else > static int > lxcDomainInterfaceStats(virDomainPtr dom, > const char *path ATTRIBUTE_UNUSED, > struct _virDomainInterfaceStats *stats > ATTRIBUTE_UNUSED) > lxcError(VIR_ERR_NO_SUPPORT, "%s", __FUNCTION__); > return -1; > } > > If so, need I commit a v2 patch for this? Yes, just remove that dummy function entirely. Regards, Daniel