[libvirt] [PATCH 02/26] remote_driver: Resolve Coverity RESOURCE_LEAK

[John Ferlan]

On 09/11/2014 11:55 AM, Eric Blake wrote:
> On 09/11/2014 02:38 AM, Peter Krempa wrote:
>> On 09/05/14 00:26, John Ferlan wrote:
>>> Since 98b9acf5aa02551dd37d0209339aba2e22e4004a
>>>
>>> This ends up being a false positive for two reasons...
>>>
>>> expected to be already allocated and thus is passed by value; whereas,
>>> the call into remoteDomainGetJobStats() 'params' is passed by reference.
>>> Thus if the VIR_ALLOC is done there is no way for it to be leaked for
>>> callers that passed by value.
>>>
>>> path that handles 'nparams == 0 && params == NULL' on entry. Thus all
> 
> Careful, my virDomainBlockCopy API passes nparams==0 && params ==
> (non-NULL 0-length array) from the RPC daemon/remote.c receiver into the
> libvirtd side of the API call.  It tripped me up the first time I tried
> to assert that nparams==0 implied params==NULL, and failed the test.
> 

Well in general the newer Coverity has been squawking about this case -
that is assumptions where nparams/params type parameters are used. In
most cases, it's a "for (i = 0; i < nparams; i++)" do something with
"params[i]" that get flagged on the params[i] reference because there's
no nparams == 0 type check.

Particular to this query though the code in question is
remoteDeserializeTypedParameters(); whereas, the remoteDomainBlockCopy()
uses remoteSerializeTypedParameters().

And yes, it's all very tricky. While I do believe from reading the code
that this particular case is a false positive - I really was trying to
find a way around making too many changes.  Open to suggestions naturally!

John


>>> other callers have guaranteed that 'params' is non NULL. Of course
>>> Coverity isn't wise enough to pick up on this, but nonetheless is
>>> does point out something "future callers" for which future callers
>>> need to be aware.
>>>
>>> Even though it is a false positive, it's probably a good idea to at
>>> least make some sort of check (and to "trick" Coverity into believing
>>> we know what we're doing).  The easiest/cheapest way was to check
>>> the input 'limit' value.  For the remoteDomainGetJobStats() it is
>>> passed as 0 indicating (perhaps) that the caller has done the
>>> limits length checking already and that its caller can handle
>>> allocating something that can be passed back to the caller.
> 
>>
>> This unfortunately breaks the remote driver impl of GetAllDomainStats.
>> As it seems that the limit parameter isn't used for automatically
>> allocated arrays and I expected that it is I'll need either fix the
>> remote impl of the stats function or add support for checking the limit
>> here. And I probably prefer option 2, fixing
>> remoteDeserializeTypedParameters to use limit correctly even for
>> auto-alloced typed parameters.
> 
> I haven't looked closely at the patch, but it is a tricky area of code.
>