On 27.10.2011 12:39, 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@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.*
The only way for nmounts being nonzero is mounts being non NULL;
Thus, if mounts is NULL, there is no way for nmounts being something
else than 0; Hence, the body of this for will not be executed.
I think the code is good as-is.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list