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

Re: [libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"



Daniel Veillard wrote:
> On Wed, Sep 02, 2009 at 11:58:07AM +0200, Jim Meyering wrote:
>> Here's the test just before the else-if in the patch below:
>>
>>     if (conn &&
>>         conn->driver &&
>>         STREQ (conn->driver->name, "remote")) {
>>
>> So, in the else-branch, "conn" is guaranteed to be NULL.
>> And dereferenced.
>>
>> This may be only a theoretical risk, but if so,
>> the test of "conn" above should be changed to an assertion,
>> and/or the parameter should get the nonnull attribute.
>
>   I'm fine with your patch, so that even if code around changes
> having a gard is IMHO a good thing

Thanks for the quick review.
Daniel Berrange said he'd prefer the nonnull approach I mentioned above,
so I've just adjusted (caveat, still not tested or even compiled)

>From 375bbb2d85fdf708858d726b6c7682e40dc07c2c Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Wed, 2 Sep 2009 12:20:32 +0200
Subject: [PATCH 1/2] infra: define ATTRIBUTE_NONNULL to mark non-NULL parameters

* src/internal.h (ATTRIBUTE_NONNULL): Define.
---
 src/internal.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/internal.h b/src/internal.h
index 936cd03..8fa579c 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -116,6 +116,14 @@
 #endif
 #endif

+#ifndef ATTRIBUTE_NONNULL
+# if __GNUC_PREREQ (3, 3)
+#  define ATTRIBUTE_NONNULL(m) __attribute__((__nonnull__(m)))
+# else
+#  define ATTRIBUTE_NONNULL(m)
+# endif
+#endif
+
 #else
 #ifndef ATTRIBUTE_UNUSED
 #define ATTRIBUTE_UNUSED
--
1.6.4.2.395.ge3d52


>From 61dc45fcae8b17d9f619563d427f1bc74f44d553 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering redhat com>
Date: Wed, 2 Sep 2009 12:22:14 +0200
Subject: [PATCH 2/2] remote_internal.c: appease clang

* src/remote_internal.c (remoteNetworkOpen): Mark "conn" parameter
as non-NULL.  Remove now-unnecessary "conn == NULL" test.
(remoteDevMonOpen): Likewise.
---
 src/remote_internal.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/remote_internal.c b/src/remote_internal.c
index ea50c11..fe36ddd 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -3281,13 +3281,12 @@ done:
 static virDrvOpenStatus
 remoteNetworkOpen (virConnectPtr conn,
                    virConnectAuthPtr auth,
-                   int flags)
+                   int flags) ATTRIBUTE_NONNULL (1)
 {
     if (inside_daemon)
         return VIR_DRV_OPEN_DECLINED;

-    if (conn &&
-        conn->driver &&
+    if (conn->driver &&
         STREQ (conn->driver->name, "remote")) {
         struct private_data *priv;

@@ -5130,13 +5129,12 @@ done:
 static virDrvOpenStatus
 remoteDevMonOpen(virConnectPtr conn,
                  virConnectAuthPtr auth ATTRIBUTE_UNUSED,
-                 int flags ATTRIBUTE_UNUSED)
+                 int flags ATTRIBUTE_UNUSED) ATTRIBUTE_NONNULL (1)
 {
     if (inside_daemon)
         return VIR_DRV_OPEN_DECLINED;

-    if (conn &&
-        conn->driver &&
+    if (conn->driver &&
         STREQ (conn->driver->name, "remote")) {
         struct private_data *priv = conn->privateData;
         /* If we're here, the remote driver is already
--
1.6.4.2.395.ge3d52


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