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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]