[libvirt] [PATCH 1/9] remote_driver: Resolve Coverity RESOURCE_LEAK
Peter Krempa
pkrempa at redhat.com
Fri Sep 12 08:38:29 UTC 2014
On 09/12/14 02:05, 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.
>
> Doing this means an adjustment to remoteConnectGetAllDomainStats()
> in order to do the prerequisite maximum checking per set of stats
> returned and passing 0 to remoteDeserializeTypedParameters() in order
> to allocate memory into &elem->params.
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
> src/remote/remote_driver.c | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 8221683..1594c89 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;
> + }
I still dislike this part. I'd rather see limit implemented also for
this branch and not used to cover up a false positive of coverity.
If they decide to fix it later on we will have a piece of code for no
reason or possibly worse if they will come up with a different error.
> if (VIR_ALLOC_N(*params, ret_params_len) < 0)
> goto cleanup;
> }
Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140912/3f5e2d76/attachment-0001.sig>
More information about the libvir-list
mailing list