[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Thu, Oct 27, 2011 at 06:39:57PM +0800, Alex Jia wrote:
> 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 redhat com wrote:
> >>>>>>From: Alex Jia<ajia redhat com>
> >>>>>>
> >>>>>>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<ajia redhat com>
> >>>>>>---
> >>>>>>  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]);

There's no need.  If  nmounts > 0, then we know mounts != NULL.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]