[libvirt] [PATCH 1/3] qemu: agent: fix uninitialized var case in qemuAgentGetFSInfo

[John Ferlan]

On 12/09/2016 02:27 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 08.12.2016 19:38, John Ferlan wrote:
>>
>>
>> On 11/24/2016 04:19 AM, Nikolay Shirokovskiy wrote:
>>> In case of 0 filesystems *info is not set while according
>>> to virDomainGetFSInfo contract user should call free on it even
>>> in case of 0 filesystems. Thus we need to properly set
>>> it. NULL will be enough as free eats NULLs ok.
>>> ---
>>>  src/qemu/qemu_agent.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
>>> index ec8d47e..c5cf403 100644
>>> --- a/src/qemu/qemu_agent.c
>>> +++ b/src/qemu/qemu_agent.c
>>> @@ -1872,6 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
>>>      ndata = virJSONValueArraySize(data);
>>>      if (!ndata) {
>>>          ret = 0;
>>> +        *info = NULL;
>>
>> ACK - although there are more ways above this hunk that allow us to get
>> to cleanup without setting *info = NULL;  Currently each of the callers
> 
> These are error paths. The caller have no obligations to free info in these cases.
> 
>> sets the input info to NULL before calling here
> 
> qemuDomainGetFSInfo does not set...
> 
>>
>> IOW: We could also move that *info = NULL up before the call to
>> virAgentMakeCommand
>>
> 
> I looked here and there in the libvirt code and found out that it does not zero out 
> output parameter immediately. I guess it makes sense. Output parameter can be 
> actually filled in deep below the call stack. Thus if one starts with convention 
> to zero out immediately one have to do it in every function on the path.

I have a recollection of having it mentioned for one of my code reviews,
hence why setting *info = NULL unconditionally early on has stuck with
me...  See virDomainGetIOThreadInfo.

Ironically virDomainGetFSInfo also has a *info = NULL prior to calling
domainGetFSInfo which calls into qemuDomainGetFSInfo.

As for testQemuAgentGetFSInfo (the other consumer of
qemuAgentGetFSInfo), it too has a qemuAgentFsInfoPtr *info = NULL; at
the top so well it should be happy, except that I see/note
qemuAgentGetFSInfo is called a second time, but in this case it expects
some sort of failure (it's not all that clear what's being tested, but
that's a different issue).  In any case, prior to that call info was not
freed nor set to NULL (maybe that's the test case - I didn't think too
long and hard about it).

In any case, whether you decide in your v2 to put the *info at the top
or keep it where it is - doesn't matter to retain the ACK.

John

> 
> I guess caller itself can zero out output parameter to simplify its cleanup logic.
> And some callers are not zero out, they cleanup conditionally for example - these 
> rely on function to properly set output parameter.
> 
> Nikolay
>