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(a)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(a)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