[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]