Re: [libvirt] [PATCH] lxc: remove redundant check

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]);