[PATCH 1/9] remote_driver: Resolve Coverity RESOURCE_LEAK

[John Ferlan <jferlan@xxxxxxxxxx>]

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@xxxxxxxxxx>
---
 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;
+        }
         if (VIR_ALLOC_N(*params, ret_params_len) < 0)
             goto cleanup;
     }
@@ -7771,6 +7783,12 @@ remoteConnectGetAllDomainStats(virConnectPtr conn,
     for (i = 0; i < ret.retStats.retStats_len; i++) {
         remote_domain_stats_record *rec = ret.retStats.retStats_val + i;
 
+        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;
+        }
+
         if (VIR_ALLOC(elem) < 0)
             goto cleanup;
 
@@ -7779,7 +7797,7 @@ remoteConnectGetAllDomainStats(virConnectPtr conn,
 
         if (remoteDeserializeTypedParameters(rec->params.params_val,
                                              rec->params.params_len,
-                                             REMOTE_CONNECT_GET_ALL_DOMAIN_STATS_MAX,
+                                             0,
                                              &elem->params,
                                              &elem->nparams))
             goto cleanup;
-- 
1.9.3

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