[libvirt] [PATCH] remote: avoid null dereference on error
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
On 05/03/2011 11:28 AM, Eric Blake wrote:
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.
I meant to add: Regression introduced in commit 158ba873. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, May 03, 2011 at 11:28:30AM -0600, Eric Blake wrote:
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(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 05/04/2011 10:38 AM, Daniel P. Berrange wrote:
On Tue, May 03, 2011 at 11:28:30AM -0600, Eric Blake wrote:
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(-)
ACK
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake