[libvirt] [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
When "domain" is NULL, don't deref NULL. Instead, just return -1, as in many other functions in this file, and as this function did up until a month ago. An alternative (taken 3 times in this file) is to do this: virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1; I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 26 Jan 2010 20:17:07 +0100 Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt to diagnose an unlikely NULL-domain or NULL-domain->conn error. --- src/xen/xen_hypervisor.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 6d8accc..0257be2 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1,7 +1,7 @@ /* * xen_internal.c: direct access to Xen hypervisor level * - * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2005-2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i; - if (domain == NULL || domain->conn == NULL) { - virXenErrorFunc (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__, - "invalid argument", 0); + if (domain == NULL || domain->conn == NULL) return -1; - } priv = (xenUnifiedPrivatePtr) domain->conn->privateData; if (priv->handle < 0 || (domain->id < 0) || -- 1.7.0.rc0.140.gfbe7
On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
When "domain" is NULL, don't deref NULL. Instead, just return -1, as in many other functions in this file, and as this function did up until a month ago.
An alternative (taken 3 times in this file) is to do this:
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1;
I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 26 Jan 2010 20:17:07 +0100 Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt to diagnose an unlikely NULL-domain or NULL-domain->conn error. --- src/xen/xen_hypervisor.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 6d8accc..0257be2 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1,7 +1,7 @@ /* * xen_internal.c: direct access to Xen hypervisor level * - * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2005-2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i;
- if (domain == NULL || domain->conn == NULL) { - virXenErrorFunc (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__, - "invalid argument", 0); + if (domain == NULL || domain->conn == NULL) return -1; - }
I'd rather we just got rid of that check completely - its a left over from a time when Xen was the only driver & these entry points needed to do full checking. Now all mandatory parameters are checked in the previous libvirt.c layer. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
When "domain" is NULL, don't deref NULL. Instead, just return -1, as in many other functions in this file, and as this function did up until a month ago.
An alternative (taken 3 times in this file) is to do this:
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1;
I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 26 Jan 2010 20:17:07 +0100 Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt to diagnose an unlikely NULL-domain or NULL-domain->conn error. --- src/xen/xen_hypervisor.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 6d8accc..0257be2 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1,7 +1,7 @@ /* * xen_internal.c: direct access to Xen hypervisor level * - * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2005-2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i;
- if (domain == NULL || domain->conn == NULL) { - virXenErrorFunc (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__, - "invalid argument", 0); + if (domain == NULL || domain->conn == NULL) return -1; - }
I'd rather we just got rid of that check completely - its a left over from a time when Xen was the only driver & these entry points needed to do full checking. Now all mandatory parameters are checked in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the "domain == NULL" tests. I want to keep this global "clean-up" patch separate from the above bug-fixing one. I'm less confident about removing the daomin->conn tests, and would be inclined to leave them as-is, or use an assert, if you want to remove them. If we also remove the daomin->conn == NULL tests, an added "assert" is the best way to keep clang/coverity from complaining about a possible NULL-dereference.
From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 27 Jan 2010 13:34:03 +0100 Subject: [PATCH] xen_hypervisor.c: remove all "domain == NULL" tests, ...
* src/xen/xen_hypervisor.c: Remove all "domain == NULL" tests. * src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to mark each "domain" parameter as "known always to be non-NULL". --- src/xen/xen_hypervisor.c | 28 ++++++++++++++-------------- src/xen/xen_hypervisor.h | 44 +++++++++++++++++++++++++++++--------------- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 0257be2..994f5ef 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1130,7 +1130,7 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams) char *schedulertype = NULL; xenUnifiedPrivatePtr priv; - if ((domain == NULL) || (domain->conn == NULL)) { + if (domain->conn == NULL) { virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return NULL; @@ -1214,7 +1214,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, { xenUnifiedPrivatePtr priv; - if ((domain == NULL) || (domain->conn == NULL)) { + if (domain->conn == NULL) { virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1; @@ -1317,7 +1317,7 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain, xenUnifiedPrivatePtr priv; char buf[256]; - if ((domain == NULL) || (domain->conn == NULL)) { + if (domain->conn == NULL) { virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1; @@ -3062,12 +3062,12 @@ xenHypervisorGetDomMaxMemory(virConnectPtr conn, int id) * * Returns the memory size in kilobytes or 0 in case of error. */ -static unsigned long +static unsigned long ATTRIBUTE_NONNULL (1) xenHypervisorGetMaxMemory(virDomainPtr domain) { xenUnifiedPrivatePtr priv; - if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return 0; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3176,7 +3176,7 @@ xenHypervisorGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info) { xenUnifiedPrivatePtr priv; - if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3284,7 +3284,7 @@ xenHypervisorPauseDomain(virDomainPtr domain) int ret; xenUnifiedPrivatePtr priv; - if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3311,7 +3311,7 @@ xenHypervisorResumeDomain(virDomainPtr domain) int ret; xenUnifiedPrivatePtr priv; - if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3338,7 +3338,7 @@ xenHypervisorDestroyDomain(virDomainPtr domain) int ret; xenUnifiedPrivatePtr priv; - if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3366,7 +3366,7 @@ xenHypervisorSetMaxMemory(virDomainPtr domain, unsigned long memory) int ret; xenUnifiedPrivatePtr priv; - if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3397,7 +3397,7 @@ xenHypervisorSetVcpus(virDomainPtr domain, unsigned int nvcpus) int ret; xenUnifiedPrivatePtr priv; - if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3429,7 +3429,7 @@ xenHypervisorPinVcpu(virDomainPtr domain, unsigned int vcpu, int ret; xenUnifiedPrivatePtr priv; - if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3475,7 +3475,7 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i; - if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3548,7 +3548,7 @@ xenHypervisorGetVcpuMax(virDomainPtr domain) int maxcpu; xenUnifiedPrivatePtr priv; - if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 5971a90..4504733 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -1,7 +1,7 @@ /* * xen_internal.h: internal API for direct access to Xen hypervisor level * - * Copyright (C) 2005 Red Hat, Inc. + * Copyright (C) 2005, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -58,48 +58,62 @@ int xenHypervisorListDomains (virConnectPtr conn, int maxids); int xenHypervisorGetMaxVcpus (virConnectPtr conn, const char *type); -int xenHypervisorDestroyDomain (virDomainPtr domain); -int xenHypervisorResumeDomain (virDomainPtr domain); -int xenHypervisorPauseDomain (virDomainPtr domain); +int xenHypervisorDestroyDomain (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); +int xenHypervisorResumeDomain (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); +int xenHypervisorPauseDomain (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetDomainInfo (virDomainPtr domain, - virDomainInfoPtr info); + virDomainInfoPtr info) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetDomInfo (virConnectPtr conn, int id, virDomainInfoPtr info); int xenHypervisorSetMaxMemory (virDomainPtr domain, - unsigned long memory); + unsigned long memory) + ATTRIBUTE_NONNULL (1); int xenHypervisorCheckID (virConnectPtr conn, int id); int xenHypervisorSetVcpus (virDomainPtr domain, - unsigned int nvcpus); + unsigned int nvcpus) + ATTRIBUTE_NONNULL (1); int xenHypervisorPinVcpu (virDomainPtr domain, unsigned int vcpu, unsigned char *cpumap, - int maplen); + int maplen) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetVcpus (virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, unsigned char *cpumaps, - int maplen); -int xenHypervisorGetVcpuMax (virDomainPtr domain); + int maplen) + ATTRIBUTE_NONNULL (1); +int xenHypervisorGetVcpuMax (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); char * xenHypervisorGetSchedulerType (virDomainPtr domain, - int *nparams); + int *nparams) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetSchedulerParameters(virDomainPtr domain, virSchedParameterPtr params, - int *nparams); + int *nparams) + ATTRIBUTE_NONNULL (1); int xenHypervisorSetSchedulerParameters(virDomainPtr domain, virSchedParameterPtr params, - int nparams); + int nparams) + ATTRIBUTE_NONNULL (1); int xenHypervisorDomainBlockStats (virDomainPtr domain, const char *path, - struct _virDomainBlockStats *stats); + struct _virDomainBlockStats *stats) + ATTRIBUTE_NONNULL (1); int xenHypervisorDomainInterfaceStats (virDomainPtr domain, const char *path, - struct _virDomainInterfaceStats *stats); + struct _virDomainInterfaceStats *stats) + ATTRIBUTE_NONNULL (1); int xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, -- 1.7.0.rc0.140.gfbe7
On Wed, Jan 27, 2010 at 01:39:13PM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
On Tue, Jan 26, 2010 at 08:24:25PM +0100, Jim Meyering wrote:
When "domain" is NULL, don't deref NULL. Instead, just return -1, as in many other functions in this file, and as this function did up until a month ago.
An alternative (taken 3 times in this file) is to do this:
virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1;
I could go either way.
From 177556167775b806a29bcb1af7ba4294d1909912 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Tue, 26 Jan 2010 20:17:07 +0100 Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt to diagnose an unlikely NULL-domain or NULL-domain->conn error. --- src/xen/xen_hypervisor.c | 7 ++----- 1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 6d8accc..0257be2 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1,7 +1,7 @@ /* * xen_internal.c: direct access to Xen hypervisor level * - * Copyright (C) 2005, 2006, 2007, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2005-2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i;
- if (domain == NULL || domain->conn == NULL) { - virXenErrorFunc (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__, - "invalid argument", 0); + if (domain == NULL || domain->conn == NULL) return -1; - }
I'd rather we just got rid of that check completely - its a left over from a time when Xen was the only driver & these entry points needed to do full checking. Now all mandatory parameters are checked in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the "domain == NULL" tests. I want to keep this global "clean-up" patch separate from the above bug-fixing one.
I'm less confident about removing the daomin->conn tests, and would be inclined to leave them as-is, or use an assert, if you want to remove them. If we also remove the daomin->conn == NULL tests, an added "assert" is the best way to keep clang/coverity from complaining about a possible NULL-dereference.
From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 27 Jan 2010 13:34:03 +0100 Subject: [PATCH] xen_hypervisor.c: remove all "domain == NULL" tests, ...
* src/xen/xen_hypervisor.c: Remove all "domain == NULL" tests. * src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to mark each "domain" parameter as "known always to be non-NULL". --- src/xen/xen_hypervisor.c | 28 ++++++++++++++-------------- src/xen/xen_hypervisor.h | 44 +++++++++++++++++++++++++++++--------------- 2 files changed, 43 insertions(+), 29 deletions(-)
diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 0257be2..994f5ef 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -1130,7 +1130,7 @@ xenHypervisorGetSchedulerType(virDomainPtr domain, int *nparams) char *schedulertype = NULL; xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) { + if (domain->conn == NULL) { virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return NULL; @@ -1214,7 +1214,7 @@ xenHypervisorGetSchedulerParameters(virDomainPtr domain, { xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) { + if (domain->conn == NULL) { virXenErrorFunc(NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1; @@ -1317,7 +1317,7 @@ xenHypervisorSetSchedulerParameters(virDomainPtr domain, xenUnifiedPrivatePtr priv; char buf[256];
- if ((domain == NULL) || (domain->conn == NULL)) { + if (domain->conn == NULL) { virXenErrorFunc (NULL, VIR_ERR_INTERNAL_ERROR, __FUNCTION__, "domain or conn is NULL", 0); return -1; @@ -3062,12 +3062,12 @@ xenHypervisorGetDomMaxMemory(virConnectPtr conn, int id) * * Returns the memory size in kilobytes or 0 in case of error. */ -static unsigned long +static unsigned long ATTRIBUTE_NONNULL (1) xenHypervisorGetMaxMemory(virDomainPtr domain) { xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return 0;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3176,7 +3176,7 @@ xenHypervisorGetDomainInfo(virDomainPtr domain, virDomainInfoPtr info) { xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3284,7 +3284,7 @@ xenHypervisorPauseDomain(virDomainPtr domain) int ret; xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3311,7 +3311,7 @@ xenHypervisorResumeDomain(virDomainPtr domain) int ret; xenUnifiedPrivatePtr priv;
- if ((domain == NULL) || (domain->conn == NULL)) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3338,7 +3338,7 @@ xenHypervisorDestroyDomain(virDomainPtr domain) int ret; xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3366,7 +3366,7 @@ xenHypervisorSetMaxMemory(virDomainPtr domain, unsigned long memory) int ret; xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3397,7 +3397,7 @@ xenHypervisorSetVcpus(virDomainPtr domain, unsigned int nvcpus) int ret; xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3429,7 +3429,7 @@ xenHypervisorPinVcpu(virDomainPtr domain, unsigned int vcpu, int ret; xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3475,7 +3475,7 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i;
- if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3548,7 +3548,7 @@ xenHypervisorGetVcpuMax(virDomainPtr domain) int maxcpu; xenUnifiedPrivatePtr priv;
- if (domain == NULL || domain->conn == NULL) + if (domain->conn == NULL) return -1;
priv = (xenUnifiedPrivatePtr) domain->conn->privateData; diff --git a/src/xen/xen_hypervisor.h b/src/xen/xen_hypervisor.h index 5971a90..4504733 100644 --- a/src/xen/xen_hypervisor.h +++ b/src/xen/xen_hypervisor.h @@ -1,7 +1,7 @@ /* * xen_internal.h: internal API for direct access to Xen hypervisor level * - * Copyright (C) 2005 Red Hat, Inc. + * Copyright (C) 2005, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -58,48 +58,62 @@ int xenHypervisorListDomains (virConnectPtr conn, int maxids); int xenHypervisorGetMaxVcpus (virConnectPtr conn, const char *type); -int xenHypervisorDestroyDomain (virDomainPtr domain); -int xenHypervisorResumeDomain (virDomainPtr domain); -int xenHypervisorPauseDomain (virDomainPtr domain); +int xenHypervisorDestroyDomain (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); +int xenHypervisorResumeDomain (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); +int xenHypervisorPauseDomain (virDomainPtr domain) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetDomainInfo (virDomainPtr domain, - virDomainInfoPtr info); + virDomainInfoPtr info) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetDomInfo (virConnectPtr conn, int id, virDomainInfoPtr info); int xenHypervisorSetMaxMemory (virDomainPtr domain, - unsigned long memory); + unsigned long memory) + ATTRIBUTE_NONNULL (1); int xenHypervisorCheckID (virConnectPtr conn, int id); int xenHypervisorSetVcpus (virDomainPtr domain, - unsigned int nvcpus); + unsigned int nvcpus) + ATTRIBUTE_NONNULL (1); int xenHypervisorPinVcpu (virDomainPtr domain, unsigned int vcpu, unsigned char *cpumap, - int maplen); + int maplen) + ATTRIBUTE_NONNULL (1); int xenHypervisorGetVcpus (virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, unsigned char *cpumaps, - int maplen); -int xenHypervisorGetVcpuMax (virDomainPtr domain); + int maplen) + ATTRIBUTE_NONNULL (1); +int xenHypervisorGetVcpuMax (virDomainPtr domain) + ATTRIBUTE_NONNULL (1);
char * xenHypervisorGetSchedulerType (virDomainPtr domain, - int *nparams); + int *nparams) + ATTRIBUTE_NONNULL (1);
int xenHypervisorGetSchedulerParameters(virDomainPtr domain, virSchedParameterPtr params, - int *nparams); + int *nparams) + ATTRIBUTE_NONNULL (1);
int xenHypervisorSetSchedulerParameters(virDomainPtr domain, virSchedParameterPtr params, - int nparams); + int nparams) + ATTRIBUTE_NONNULL (1);
int xenHypervisorDomainBlockStats (virDomainPtr domain, const char *path, - struct _virDomainBlockStats *stats); + struct _virDomainBlockStats *stats) + ATTRIBUTE_NONNULL (1); int xenHypervisorDomainInterfaceStats (virDomainPtr domain, const char *path, - struct _virDomainInterfaceStats *stats); + struct _virDomainInterfaceStats *stats) + ATTRIBUTE_NONNULL (1);
int xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long long *freeMems, --
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
Subject: [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument
* src/xen/xen_hypervisor.c (xenHypervisorGetVcpus): Don't attempt to diagnose an unlikely NULL-domain or NULL-domain->conn error. ... @@ -3475,11 +3475,8 @@ xenHypervisorGetVcpus(virDomainPtr domain, virVcpuInfoPtr info, int maxinfo, virVcpuInfoPtr ipt; int nbinfo, i;
- if (domain == NULL || domain->conn == NULL) { - virXenErrorFunc (domain->conn, VIR_ERR_INVALID_ARG, __FUNCTION__, - "invalid argument", 0); + if (domain == NULL || domain->conn == NULL) return -1; - }
I'd rather we just got rid of that check completely - its a left over from a time when Xen was the only driver & these entry points needed to do full checking. Now all mandatory parameters are checked in the previous libvirt.c layer.
Here's an additional patch, to eliminate all of the "domain == NULL" tests. I want to keep this global "clean-up" patch separate from the above bug-fixing one.
I'm less confident about removing the daomin->conn tests, and would be inclined to leave them as-is, or use an assert, if you want to remove them. If we also remove the daomin->conn == NULL tests, an added "assert" is the best way to keep clang/coverity from complaining about a possible NULL-dereference.
From 9e6f7ca7a0dfa6b220e598d04ca40d33e67feb22 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 27 Jan 2010 13:34:03 +0100 Subject: [PATCH] xen_hypervisor.c: remove all "domain == NULL" tests, ...
* src/xen/xen_hypervisor.c: Remove all "domain == NULL" tests. * src/xen/xen_hypervisor.h: Instead, use ATTRIBUTE_NONNULL to mark each "domain" parameter as "known always to be non-NULL". ... ACK
Thanks. I'll push those as soon as "make distcheck" etc. passes.
participants (2)
-
Daniel P. Berrange -
Jim Meyering