[libvirt] [PATCH] More paranoid initialization of 'nparams' variable in dispatch code
From: "Daniel P. Berrange" <berrange@redhat.com> Since the 'nparams' variable passed to virTypedParametersFree is supposed to represent the size of the 'params' array, it is bad practice to initialize it to a non-zero value, until the array has been allocated. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 78 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 34 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index e5e3f2c..c5567f4 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1001,7 +1001,7 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS { virDomainPtr dom = NULL; virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; int rv = -1; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -1011,12 +1011,13 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS goto cleanup; } - if (nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) goto no_memory; + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -1109,7 +1110,7 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE { virDomainPtr dom = NULL; virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; int rv = -1; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -1119,12 +1120,13 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE goto cleanup; } - if (nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) goto no_memory; + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -1284,7 +1286,7 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, virTypedParameterPtr params = NULL; virDomainPtr dom = NULL; const char *path = args->path; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -1299,14 +1301,15 @@ remoteDispatchDomainBlockStatsFlags(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; flags = args->flags; - if (nparams > REMOTE_DOMAIN_BLOCK_STATS_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_BLOCK_STATS_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (virDomainBlockStatsFlags(dom, path, params, &nparams, flags) < 0) goto cleanup; @@ -1913,7 +1916,7 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, { virDomainPtr dom = NULL; virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -1926,14 +1929,15 @@ remoteDispatchDomainGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - if (nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -1977,7 +1981,7 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, { virDomainPtr dom = NULL; virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -1990,14 +1994,15 @@ remoteDispatchDomainGetNumaParameters(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - if (nparams > REMOTE_DOMAIN_NUMA_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_NUMA_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -2041,7 +2046,7 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, { virDomainPtr dom = NULL; virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -2054,14 +2059,15 @@ remoteDispatchDomainGetBlkioParameters(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - if (nparams > REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -2106,7 +2112,7 @@ remoteDispatchNodeGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, virNodeCPUStatsPtr params = NULL; int i; int cpuNum = args->cpuNum; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -2119,14 +2125,15 @@ remoteDispatchNodeGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - if (nparams > REMOTE_NODE_CPU_STATS_MAX) { + if (args->nparams > REMOTE_NODE_CPU_STATS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (virNodeGetCPUStats(priv->conn, cpuNum, params, &nparams, flags) < 0) goto cleanup; @@ -2184,7 +2191,7 @@ remoteDispatchNodeGetMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED, virNodeMemoryStatsPtr params = NULL; int i; int cellNum = args->cellNum; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -2197,14 +2204,15 @@ remoteDispatchNodeGetMemoryStats(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - if (nparams > REMOTE_NODE_MEMORY_STATS_MAX) { + if (args->nparams > REMOTE_NODE_MEMORY_STATS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (virNodeGetMemoryStats(priv->conn, cellNum, params, &nparams, flags) < 0) goto cleanup; @@ -2303,7 +2311,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, virDomainPtr dom = NULL; int rv = -1; virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); @@ -2312,15 +2320,16 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, goto cleanup; } - if (nparams > REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -3800,7 +3809,7 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS virDomainPtr dom = NULL; virTypedParameterPtr params = NULL; const char *device = args->device; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -3813,14 +3822,15 @@ remoteDispatchDomainGetInterfaceParameters(virNetServerPtr server ATTRIBUTE_UNUS flags = args->flags; - if (nparams > REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX) { + if (args->nparams > REMOTE_DOMAIN_INTERFACE_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } + nparams = args->nparams; if (!(dom = get_nonnull_domain(priv->conn, args->dom))) goto cleanup; @@ -4508,7 +4518,7 @@ remoteDispatchNodeGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, remote_node_get_memory_parameters_ret *ret) { virTypedParameterPtr params = NULL; - int nparams = args->nparams; + int nparams = 0; unsigned int flags; int rv = -1; struct daemonClientPrivate *priv = @@ -4521,15 +4531,15 @@ remoteDispatchNodeGetMemoryParameters(virNetServerPtr server ATTRIBUTE_UNUSED, flags = args->flags; - if (nparams > REMOTE_NODE_MEMORY_PARAMETERS_MAX) { + if (args->nparams > REMOTE_NODE_MEMORY_PARAMETERS_MAX) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); goto cleanup; } - if (nparams && VIR_ALLOC_N(params, nparams) < 0) { + if (args->nparams && VIR_ALLOC_N(params, args->nparams) < 0) { virReportOOMError(); goto cleanup; } - + nparams = args->nparams; if (virNodeGetMemoryParameters(priv->conn, params, &nparams, flags) < 0) goto cleanup; -- 1.8.2.1
On 05/02/2013 05:39 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Since the 'nparams' variable passed to virTypedParametersFree is supposed to represent the size of the 'params' array, it is bad practice to initialize it to a non-zero value, until the array has been allocated.
Thankfully, virTypedParamsFree() does nothing when passed a NULL params pointer, regardless of the size of nparams, so we had no real bug here; but I agree that it is wiser to be robust against any future changes in the code.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/remote.c | 78 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 34 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake