Re: [PATCH] lxc: remove redundant check

[Alex Jia <ajia@xxxxxxxxxx>]

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@xxxxxxxxxx wrote:

From: Alex Jia<ajia@xxxxxxxxxx>

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

Daniel, please correct me if I'm wrong again :-)

Thanks a lot!
Alex

Regards,
Daniel

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list