On 12/2/20 9:44 AM, Ján Tomko wrote:
> The commit summary uses 'and' which makes me think there are two
> issues, but the commit message and code only seem to fix one.
>
True - I changed my mind multiple times on this one w/r/t how involved
the fix should be. Originally I did have cleanup added, but then changed
my mind to allow the caller to do it and pass &niothreads instead.
I can rework this one - thanks for the reviews/pushes. I'll also drop
the pidfilefd one and keep it in my false positive list.
Tks -
John
> On a Wednesday in 2020, John Ferlan wrote:
>> If qemuDomainGetIOThreadsMon fails because qemuDomainObjExitMonitor
>> fails, then a -1 is returned which overwrites @niothreads causing a
>> memory leak. Let's pass @niothreads instead.
>>
>> Found by Coverity.
>>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>> src/qemu/qemu_driver.c | 19 +++++++++----------
>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 8eaa3ce68f..870159de47 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4972,17 +4972,16 @@ qemuDomainGetMaxVcpus(virDomainPtr dom)
>> static int
>> qemuDomainGetIOThreadsMon(virQEMUDriverPtr driver,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â virDomainObjPtr vm,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â qemuMonitorIOThreadInfoPtr **iothreads)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â qemuMonitorIOThreadInfoPtr **iothreads,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int *niothreads)
>
> Returning niothreads separately from success/error might clear things
> up,
>
>> {
>> Â Â Â qemuDomainObjPrivatePtr priv = vm->privateData;
>> -Â Â Â int niothreads = 0;
>>
>> Â Â Â qemuDomainObjEnterMonitor(driver, vm);
>> -Â Â Â niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
>> -Â Â Â if (qemuDomainObjExitMonitor(driver, vm) < 0 || niothreads < 0)
>> +Â Â Â *niothreads = qemuMonitorGetIOThreads(priv->mon, iothreads);
>
> but qemuMonitorGetIOThreads can also return -1. In that case we
> should not:
> a) return 0 in this function
> b) compare the return value against unsigned size_t in the cleanup
> Â section of qemuDomainGetStatsIOThread
>
>> +Â Â Â if (qemuDomainObjExitMonitor(driver, vm) < 0)
>
> I think that now that we take a job even for destroying the domain
> in processMonitorEOFEvent, this should not happen.
>
> But if it still can, it would be more consistent if
> qemuDomainGetIOThreadsMon
> cleaned up after itself if it returns -1, instead of leaving it up
> to the caller.
>
> (Other callers of qemuMonitorGetIOThreads and qemuDomainGetIOThreadsMon
> Â seem to handle it properly and check if (iothreads) in their cleanup
> Â sections, it's only qemuDomainGetStatsIOThread that relies on
> Â niothreads being right)
>
> Jano
>
>> Â Â Â Â Â Â Â return -1;
>> -
>> -Â Â Â return niothreads;
>> +Â Â Â return 0;
>> }
>>
>>
>> @@ -5014,7 +5013,7 @@ qemuDomainGetIOThreadsLive(virQEMUDriverPtr driver,
>> Â Â Â Â Â Â Â goto endjob;
>> Â Â Â }
>>
>> -Â Â Â if ((niothreads = qemuDomainGetIOThreadsMon(driver, vm,
>> &iothreads)) < 0)
>> +Â Â Â if (qemuDomainGetIOThreadsMon(driver, vm, &iothreads,
>> &niothreads) < 0)
>> Â Â Â Â Â Â Â goto endjob;
>>
>> Â Â Â /* Nothing to do */
>> @@ -18507,7 +18506,7 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr
>> driver,
>> Â Â Â qemuDomainObjPrivatePtr priv = dom->privateData;
>> Â Â Â size_t i;
>> Â Â Â qemuMonitorIOThreadInfoPtr *iothreads = NULL;
>> -Â Â Â int niothreads;
>> +Â Â Â int niothreads = 0;
>> Â Â Â int ret = -1;
>>
>> Â Â Â if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
>> @@ -18516,8 +18515,8 @@ qemuDomainGetStatsIOThread(virQEMUDriverPtr
>> driver,
>> Â Â Â if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD))
>> Â Â Â Â Â Â Â return 0;
>>
>> -Â Â Â if ((niothreads = qemuDomainGetIOThreadsMon(driver, dom,
>> &iothreads)) < 0)
>> -Â Â Â Â Â Â Â return -1;
>> +Â Â Â if (qemuDomainGetIOThreadsMon(driver, dom, &iothreads,
>> &niothreads) < 0)
>> +Â Â Â Â Â Â Â goto cleanup;
>>
>> Â Â Â /* qemuDomainGetIOThreadsMon returns a NULL-terminated list, so we
>> must free
>> Â Â Â Â * it even if it returns 0 */
>> --Â
>> 2.28.0
>>
