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

[John Ferlan]

On 09/11/2014 04: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
>> 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.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/remote/remote_driver.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>> index 915e8e5..4b4644d 100644
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -1761,6 +1761,18 @@ remoteDeserializeTypedParameters(remote_typed_param *ret_params_val,
>>              goto cleanup;
>>          }
>>      } else {
>> +        /* For callers that can return this allocated buffer back to their
>> +         * caller, pass a 0 in the 'limit' field indicating that the
>> +         * ret_params_len MAX checking has already occurred *and* that
>> +         * the caller has passed 'params' by reference; otherwise, a
>> +         * caller that receives the 'params' by value will potentially
>> +         * leak memory we're allocating here
>> +         */
>> +        if (limit != 0) {
>> +            virReportError(VIR_ERR_RPC, "%s",
>> +                           _("invalid call - params is NULL on input"));
>> +            goto cleanup;
>> +        }
>>          if (VIR_ALLOC_N(*params, ret_params_len) < 0)
>>              goto cleanup;
>>      }
>>
> 
> 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.
> 
> Peter
> 


Hmm... yeah after a bit of digging I see - the '&ret->params' is a
virTypedParameterPtr which yes, will be NULL on input, <sigh>...  Of
course 'limit' never being checked could have led to other problems down
the road, but I don't think we're there yet.

The "good" news is it seems a comparison

if (rec->params.params_len > REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX) {
    virReportError(VIR_ERR_RPC, "%s",
                   _("returned number of parameters exceeds limit"));
    goto cleanup;
}

Prior to the (VIR_ALLOC(elem) < 0) check would suffice as well as
passing the '0' in the 3rd (or limit) param.

If we don't do the limit != 0 check, then other callers of
remoteDeserializeTypedParameters() would probably need some sort of /*
coverity[resource_leak] */ comment or the remoteDomainGetJobStats would
have to change to not pass 0 if the decision was to adjust the logic in
remoteDeserializeTypedParameters

I was merely using that 0 passing as a "marker" since 'limit' wasn't
checked/used in the auto allocated case. It was a whole lot easier,
cheaper, simpler than the alternatives.

John