[PATCH] remote: avoid null dereference on error

[Eric Blake <eblake@xxxxxxxxxx>]

Clang found three instances of uninitialized use of nparams in
the cleanup path.  Unfortunately, one is a false positive: clang
couldn't see that ret->params.params_val is guaranteed to be
NULL unless allocated within a function, and that nparams is
guaranteed to be assigned prior to the allocation; hoisting the
assignment to nparams to be earlier in the function shuts up
that false positive.  But two of the reports also happened to
highlight a real bug - the error path can dereference NULL.

* daemon/remote.c (remoteDispatchDomainGetMemoryParameters)
(remoteDispatchDomainGetBlkioParameters): Don't clear fields if
array was not allocated.
(remoteDispatchDomainGetSchedulerParameters): Initialize nparams
earlier.
---
 daemon/remote.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/daemon/remote.c b/daemon/remote.c
index eedbc77..214f7a4 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -886,7 +886,8 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
 {
     virDomainPtr dom = NULL;
     virSchedParameterPtr params = NULL;
-    int i, nparams;
+    int i;
+    int nparams = args->nparams;
     int rv = -1;

     if (!conn) {
@@ -894,8 +895,6 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
         goto cleanup;
     }

-    nparams = args->nparams;
-
     if (nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) {
         virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
         goto cleanup;
@@ -2970,7 +2969,8 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
 {
     virDomainPtr dom = NULL;
     virMemoryParameterPtr params = NULL;
-    int i, nparams;
+    int i;
+    int nparams = args->nparams;
     unsigned int flags;
     int rv = -1;

@@ -2979,7 +2979,6 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
         goto cleanup;
     }

-    nparams = args->nparams;
     flags = args->flags;

     if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) {
@@ -3060,9 +3059,11 @@ success:
 cleanup:
     if (rv < 0) {
         remoteDispatchError(rerr);
-        for (i = 0; i < nparams; i++)
-            VIR_FREE(ret->params.params_val[i].field);
-        VIR_FREE(ret->params.params_val);
+        if (ret->params.params_val) {
+            for (i = 0; i < nparams; i++)
+                VIR_FREE(ret->params.params_val[i].field);
+            VIR_FREE(ret->params.params_val);
+        }
     }
     if (dom)
         virDomainFree(dom);
@@ -3186,7 +3187,8 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
 {
     virDomainPtr dom = NULL;
     virBlkioParameterPtr params = NULL;
-    int i, nparams;
+    int i;
+    int nparams = args->nparams;
     unsigned int flags;
     int rv = -1;

@@ -3195,7 +3197,6 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
         goto cleanup;
     }

-    nparams = args->nparams;
     flags = args->flags;

     if (nparams > REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX) {
@@ -3276,9 +3277,11 @@ success:
 cleanup:
     if (rv < 0) {
         remoteDispatchError(rerr);
-        for (i = 0; i < nparams; i++)
-            VIR_FREE(ret->params.params_val[i].field);
-        VIR_FREE(ret->params.params_val);
+        if (ret->params.params_val) {
+            for (i = 0; i < nparams; i++)
+                VIR_FREE(ret->params.params_val[i].field);
+            VIR_FREE(ret->params.params_val);
+        }
     }
     VIR_FREE(params);
     if (dom)
-- 
1.7.4.4

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