[libvirt] [PATCH] remote_internal.c: don't dereference a NULL "conn"
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@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
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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
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@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@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.
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@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
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 :|
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.
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
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.
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 :|
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Jim Paris -
Paolo Bonzini