4:58 a.m.
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.
From a1b1d36d96f6b50ddf514539af85da20ca671bf5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 2 Sep 2009 11:54:38 +0200
Subject: [PATCH] remote_internal.c: don't dereference a NULL "conn"
* src/remote_internal.c (remoteDevMonOpen): Avoid NULL-dereference.
---
src/remote_internal.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/remote_internal.c b/src/remote_internal.c
index ea50c11..141fef9 100644
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -5148,7 +5148,7 @@ remoteDevMonOpen(virConnectPtr conn,
conn->devMonPrivateData = priv;
remoteDriverUnlock(priv);
return VIR_DRV_OPEN_SUCCESS;
- } else if (conn->networkDriver &&
+ } else if (conn && conn->networkDriver &&
STREQ (conn->networkDriver->name, "remote")) {
struct private_data *priv = conn->networkPrivateData;
remoteDriverLock(priv);
--
1.6.4.2.395.ge3d52
5:23 a.m.
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
ACK
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5:26 a.m.
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
5:29 a.m.
Jim Meyering wrote:
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.
...
>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.
Note that this patch now address two reported NULL-deref problems,
not just one, like the original.
5:34 a.m.
On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
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)
Yeah, for functions where it is expected that the passed in param
be non-NULL, then annotations are definitely the way togo. This
lets the compiler/checkers validate the callers instead, avoiding
the need to clutter the methods with irrelevant NULL checks.
>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
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 :|
5:51 a.m.
Daniel P. Berrange wrote:
On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
> 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)
Yeah, for functions where it is expected that the passed in param
be non-NULL, then annotations are definitely the way togo. This
lets the compiler/checkers validate the callers instead, avoiding
the need to clutter the methods with irrelevant NULL checks.
ACK
Thanks.
I'll post a revised patch once done, and built/tested.
FYI, I see there are very similar ones also in these functions:
remoteSecretOpen
remoteStorageOpen
remoteInterfaceOpen
so far, the changes all look identical.
10:48 a.m.
Daniel P. Berrange wrote:
On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
> 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)
Yeah, for functions where it is expected that the passed in param
be non-NULL, then annotations are definitely the way togo. This
lets the compiler/checkers validate the callers instead, avoiding
the need to clutter the methods with irrelevant NULL checks.
Does __attribute__((__nonnull__())) really cover the case we're
concerned about here? As I understand it, it tells the compiler
1) if the caller obviously passes NULL, emit a warning
2) assume that the arguments are non-NULL and optimize away
In other words it will do nothing to prevent a null dereference and
will even make it more likely since non-NULL checks will be skipped.
For example:
test.c:
--------
#include <stdio.h>
#include <stdlib.h>
void foo(int *a) {
if (a) printf("%d\n", *a);
else printf("(null)\n");
}
__attribute__((__nonnull__ (1)))
void bar(int *a) {
if (a) printf("%d\n", *a);
else printf("(null)\n");
}
int main(void)
{
foo(malloc(1000000000000ULL));
bar(malloc(1000000000000ULL));
return 0;
}
--------
$ gcc -O2 -Wall -Werror -o test test.c
$ ./test
(null)
Segmentation fault
$
-jim
4 a.m.
Jim Paris wrote:
Daniel P. Berrange wrote:
> On Wed, Sep 02, 2009 at 12:26:43PM +0200, Jim Meyering wrote:
> > 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)
>
> Yeah, for functions where it is expected that the passed in param
> be non-NULL, then annotations are definitely the way togo. This
> lets the compiler/checkers validate the callers instead, avoiding
> the need to clutter the methods with irrelevant NULL checks.
Does __attribute__((__nonnull__())) really cover the case we're
concerned about here? As I understand it, it tells the compiler
1) if the caller obviously passes NULL, emit a warning
2) assume that the arguments are non-NULL and optimize away
In other words it will do nothing to prevent a null dereference and
That's true, in a way.
will even make it more likely since non-NULL checks will be skipped.
Once a parameter is marked nonnull, tools can do a better job
of spotting *callers* that might mistakenly pass a corresponding NULL
argument. Without nonnull, the compiler could only detect a problem
*inside* the function, in the case that we lack adequate protection
on a dereferencing expression.
For external functions, it's a policy decision. If you say the
function is defined only for non-NULL pointers, then you may as well
add an assert(ptr != NULL) and __attribute__((nonnull...)). Otherwise,
we must perform the test at run-time. Note that memcpy, strcpy, unlink,
stat, etc. have parameters that can be marked with the nonnull attribute.
That's a plus, because compiler/analyzer tools can then warn callers
about misuse, and they needn't be burdened with detecting and diagnosing
NULL pointers.
Using attribute-nonnull is not an excuse for skipping a "ptr == NULL"
test. Rather, it is a way to tell the compiler that we require
every caller to ensure that a "ptr" parameter is non-NULL.
This has been true of most internal "conn" parameters for some time.
There were even quite a few places in the code where "conn" would be
dereferenced without first ensuring it is non-NULL, but there was no
way to trigger the NULL-deref in those seemingly-erroneous bits of code,
because upstream tests ensured non-NULL conn.
I spent time adding guards, as I would find those disturbing bits
of code. I now think that I should have been adding assertions and/or
attribute-nonnull directives, instead.
Adding an "assert (ptr != NULL);" is tempting, because then we'd get
a pretty diagnostic with filename:lineno rather than just a segfault,
whenever this assumption is violated. However, adding a raw "assert"
use in library grade code is frowned upon, to say the least.
5:59 a.m.
On Wed, Sep 02, 2009 at 11:48:21AM -0400, Jim Paris wrote:
>
> Yeah, for functions where it is expected that the passed in param
> be non-NULL, then annotations are definitely the way togo. This
> lets the compiler/checkers validate the callers instead, avoiding
> the need to clutter the methods with irrelevant NULL checks.
Does __attribute__((__nonnull__())) really cover the case we're
concerned about here? As I understand it, it tells the compiler
1) if the caller obviously passes NULL, emit a warning
2) assume that the arguments are non-NULL and optimize away
In other words it will do nothing to prevent a null dereference and
will even make it more likely since non-NULL checks will be skipped.
We should only add the nonnull attribute annotation on methods
where we are not intending to do any checks for non-NULL. They
would already be segfaulting if passed a NULL pointer, so adding
the annotation gives us the static validation of the existing
non-NULL assumption.
For example:
test.c:
--------
#include <stdio.h>
#include <stdlib.h>
void foo(int *a) {
if (a) printf("%d\n", *a);
else printf("(null)\n");
}
__attribute__((__nonnull__ (1)))
void bar(int *a) {
if (a) printf("%d\n", *a);
else printf("(null)\n");
}
int main(void)
{
foo(malloc(1000000000000ULL));
bar(malloc(1000000000000ULL));
return 0;
}
--------
$ gcc -O2 -Wall -Werror -o test test.c
$ ./test
(null)
Segmentation fault
THis is not a case where we would add a non-null annotation - the method
is clearly expecting a NULL parameter as valid since it has a check &
branch to handle that. So using the annotation is not correct.
The prime example of usefulness in libvirt is in the main API glue
- In include/libvirt.h most of the parameters are documented to
be non-NULL. We should *not*, however, annotate libvirt.h since
the implmentation of these APIs is including checks for NULL
and returns a runtime error to the caller in this case. We
don't want the compiler to optimize away these checks
- In the src/driver.h, and the implementations of these driver
APIs eg qemu_driver.c, xen_unified.c, we *should* include
the non-NULL annotation. These methods are relying on the
fact that libvirt.c has already weeded out any NULL pointers
and so don't do any NULL checks themselves.
Regards,
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 :|
5559
days inactive
5560
days old
9 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Jim Paris -
Paolo Bonzini