Re: [libvirt] [PATCH] xen_hypervisor.c: avoid NULL deref for NULL domain argument

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(a)redhat.com&gt; >> 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(a)redhat.com&gt; 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, --