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

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(a)redhat.com&gt; > --- > 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