[libvirt] [PATCH] daemon: Don't try to free an unsigned int in error paths
--- daemon/remote.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 54fef64..1c98bba 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3681,7 +3681,7 @@ remoteDispatchListNetworks(struct qemud_server *server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) { remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val); } return rv; } @@ -4200,7 +4200,7 @@ remoteDispatchListInterfaces(struct qemud_server *server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) { remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val); } return rv; } @@ -4275,7 +4275,7 @@ remoteDispatchListDefinedInterfaces(struct qemud_server *server ATTRIBUTE_UNUSED cleanup: if (rv < 0) { remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val); } return rv; } @@ -8544,7 +8544,7 @@ remoteDispatchListNwfilters(struct qemud_server *server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) { remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val); } return rv; } -- 1.7.0.4
On 04/22/2011 10:11 AM, Matthias Bolte wrote:
--- daemon/remote.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 54fef64..1c98bba 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3681,7 +3681,7 @@ remoteDispatchListNetworks(struct qemud_server *server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) { remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val);
And to think I missed those in the huge patch. Oops. ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Apr 22, 2011 at 10:17:01AM -0600, Eric Blake wrote:
On 04/22/2011 10:11 AM, Matthias Bolte wrote:
remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val);
And to think I missed those in the huge patch. Oops.
Maybe VIR_FREE should be changed to something like -# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { void *check_type = (ptr); virFree(&(check_type)); } while (0) so that the compiler can warn about such issues? With this change, I get remote.c: In function 'remoteDispatchListInterfaces': remote.c:3978:33: warning: initialization makes pointer from integer without a cast [enabled by default] I also get more warnings about casting from const to non-const, this can be avoided by making check_type const void *, but maybe these warnings indicate that something that shouldn't be freed is freed. Christophe
On 04/22/2011 10:31 AM, Christophe Fergeau wrote:
On Fri, Apr 22, 2011 at 10:17:01AM -0600, Eric Blake wrote:
On 04/22/2011 10:11 AM, Matthias Bolte wrote:
remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val);
And to think I missed those in the huge patch. Oops.
Maybe VIR_FREE should be changed to something like
-# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { void *check_type = (ptr); virFree(&(check_type)); } while (0)
Not quite. That assigns check_type to NULL, rather than the intended assignment of ptr. But the idea has merit; I'll see if I can come up with a variant that still evaluates ptr only once, by wrapping the type check in a sizeof hack.
I also get more warnings about casting from const to non-const, this can be avoided by making check_type const void *, but maybe these warnings indicate that something that shouldn't be freed is freed.
How many warnings? If it's only a handful, we should start investigating; if it's lots, then I'm not sure how much it buys us. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Fri, Apr 22, 2011 at 10:55:04AM -0600, Eric Blake wrote:
On 04/22/2011 10:31 AM, Christophe Fergeau wrote:
-# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { void *check_type = (ptr); virFree(&(check_type)); } while (0)
Not quite. That assigns check_type to NULL, rather than the intended assignment of ptr.
Ooops, indeed. Initially, I didn't change the parameter of virFree, but got "unused variable 'check_type'" warnings from gcc, so I went this way. Using ATTRIBUTE_UNUSED and not changing virFree parameter will indeed work better ;)
I also get more warnings about casting from const to non-const, this can be avoided by making check_type const void *, but maybe these warnings indicate that something that shouldn't be freed is freed.
How many warnings? If it's only a handful, we should start investigating; if it's lots, then I'm not sure how much it buys us.
I get 93 such warnings on a test build, I'll look at a few ones at random and report if that shows real bugs. Christophe
There were recently some bugs where VIR_FREE was called with an int parameter. Try to affect the value passed to VIR_FREE to a const void* so that the compiler gets a chance to emit a warning if we didn't pass a pointer to VIR_FREE. --- src/util/memory.h | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/util/memory.h b/src/util/memory.h index 66b4c42..fab425d 100644 --- a/src/util/memory.h +++ b/src/util/memory.h @@ -197,7 +197,11 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * Free the memory stored in 'ptr' and update to point * to NULL. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { \ + ATTRIBUTE_UNUSED const void *check_type = (ptr); \ + virFree(&(ptr)); \ + } while (0) # if TEST_OOM -- 1.7.4.4
On Fri, Apr 22, 2011 at 08:21:24PM +0200, Christophe Fergeau wrote:
There were recently some bugs where VIR_FREE was called with an int parameter. Try to affect the value passed to VIR_FREE to a const void* so that the compiler gets a chance to emit a warning if we didn't pass a pointer to VIR_FREE.
With this patch, I don't get any warnings except for the remote.c: In function ‘remoteDispatchListNetworks’: remote.c:3517:67: attention : initialization makes pointer from integer without a cast [enabled by default] which were just fixed. Christophe
On 04/22/2011 12:21 PM, Christophe Fergeau wrote:
There were recently some bugs where VIR_FREE was called with an int parameter. Try to affect the value passed to VIR_FREE to a const void* so that the compiler gets a chance to emit a warning if we didn't pass a pointer to VIR_FREE. --- src/util/memory.h | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/memory.h b/src/util/memory.h index 66b4c42..fab425d 100644 --- a/src/util/memory.h +++ b/src/util/memory.h @@ -197,7 +197,11 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * Free the memory stored in 'ptr' and update to point * to NULL. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { \ + ATTRIBUTE_UNUSED const void *check_type = (ptr); \ + virFree(&(ptr)); \ + } while (0)
That evaluates ptr twice. We can do better, by exploiting that the ternary operator can be used to determine the type of an expression without evaluating it. Gcc allows 1?(void*)expr:pointer (the resulting type is void*), but hates 1?(void*)expr:int (promoting to int provokes a warning): cc1: warnings being treated as errors remote.c: In function 'remoteDispatchListNetworks': remote.c:3684:70: error: pointer/integer type mismatch in conditional expression So how about: diff --git i/src/util/memory.h w/src/util/memory.h index 66b4c42..d77a295 100644 --- i/src/util/memory.h +++ w/src/util/memory.h @@ -1,7 +1,7 @@ /* * memory.c: safer memory allocation * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -197,7 +197,11 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * Free the memory stored in 'ptr' and update to point * to NULL. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +/* The ternary ensures that ptr is a pointer and not an integer type, + * while evaluating ptr only once. For now, we intentionally cast + * away const, since a number of callers safely pass const char *. + */ +# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr))) # if TEST_OOM -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/4/23 Eric Blake <eblake@redhat.com>:
On 04/22/2011 12:21 PM, Christophe Fergeau wrote:
There were recently some bugs where VIR_FREE was called with an int parameter. Try to affect the value passed to VIR_FREE to a const void* so that the compiler gets a chance to emit a warning if we didn't pass a pointer to VIR_FREE. --- src/util/memory.h | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/src/util/memory.h b/src/util/memory.h index 66b4c42..fab425d 100644 --- a/src/util/memory.h +++ b/src/util/memory.h @@ -197,7 +197,11 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * Free the memory stored in 'ptr' and update to point * to NULL. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { \ + ATTRIBUTE_UNUSED const void *check_type = (ptr); \ + virFree(&(ptr)); \ + } while (0)
That evaluates ptr twice. We can do better, by exploiting that the ternary operator can be used to determine the type of an expression without evaluating it. Gcc allows 1?(void*)expr:pointer (the resulting type is void*), but hates 1?(void*)expr:int (promoting to int provokes a warning):
cc1: warnings being treated as errors remote.c: In function 'remoteDispatchListNetworks': remote.c:3684:70: error: pointer/integer type mismatch in conditional expression
So how about:
diff --git i/src/util/memory.h w/src/util/memory.h index 66b4c42..d77a295 100644 --- i/src/util/memory.h +++ w/src/util/memory.h @@ -1,7 +1,7 @@ /* * memory.c: safer memory allocation * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -197,7 +197,11 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * Free the memory stored in 'ptr' and update to point * to NULL. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +/* The ternary ensures that ptr is a pointer and not an integer type, + * while evaluating ptr only once. For now, we intentionally cast + * away const, since a number of callers safely pass const char *. + */ +# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr)))
# if TEST_OOM
ACK, to your improved version. Matthias
On 04/24/2011 04:17 AM, Matthias Bolte wrote:
So how about:
diff --git i/src/util/memory.h w/src/util/memory.h index 66b4c42..d77a295 100644 --- i/src/util/memory.h +++ w/src/util/memory.h @@ -1,7 +1,7 @@ /* * memory.c: safer memory allocation * - * Copyright (C) 2010 Red Hat, Inc. + * Copyright (C) 2010-2011 Red Hat, Inc. * Copyright (C) 2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -197,7 +197,11 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * Free the memory stored in 'ptr' and update to point * to NULL. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +/* The ternary ensures that ptr is a pointer and not an integer type, + * while evaluating ptr only once. For now, we intentionally cast + * away const, since a number of callers safely pass const char *. + */ +# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr)))
# if TEST_OOM
ACK, to your improved version.
Pushed with this commit message: commit 90d761eeb26c9619571b68a8863b8425a33555d6 Author: Eric Blake <eblake@redhat.com> Date: Fri Apr 22 20:15:50 2011 -0600 build: make VIR_FREE do some type checking We can exploit the fact that gcc warns about int-to-pointer conversion in ternary cond?(void*):(int) in order to prevent future mistakes of calling VIR_FREE on a scalar lvalue. For example, between commits 158ba873 and 802e2df, we would have had this warning: cc1: warnings being treated as errors remote.c: In function 'remoteDispatchListNetworks': remote.c:3684:70: error: pointer/integer type mismatch in conditional expression There are still a number of places that malloc into a const char*; while it would probably be worth scrubbing them to use char* instead, that is a separate patch, so we have to cast away const in VIR_FREE for now. * src/util/memory.h (VIR_FREE): Make gcc warn about integers. Iteratively developed from a patch by Christophe Fergeau. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hey, On Fri, Apr 22, 2011 at 05:36:25PM -0600, Eric Blake wrote:
That evaluates ptr twice. We can do better, by exploiting that the ternary operator can be used to determine the type of an expression without evaluating it. Gcc allows 1?(void*)expr:pointer (the resulting type is void*), but hates 1?(void*)expr:int (promoting to int provokes a warning):
cc1: warnings being treated as errors remote.c: In function 'remoteDispatchListNetworks': remote.c:3684:70: error: pointer/integer type mismatch in conditional expression
So how about:
Sorry for the late answer, but this looks good to me too. Let's hope gcc doesn't get too clever and never stops warning about this. Thanks, Christophe
2011/4/22 Eric Blake <eblake@redhat.com>:
On 04/22/2011 10:31 AM, Christophe Fergeau wrote:
On Fri, Apr 22, 2011 at 10:17:01AM -0600, Eric Blake wrote:
On 04/22/2011 10:11 AM, Matthias Bolte wrote:
remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val);
And to think I missed those in the huge patch. Oops.
Maybe VIR_FREE should be changed to something like
-# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE(ptr) \ + do { void *check_type = (ptr); virFree(&(check_type)); } while (0)
Not quite. That assigns check_type to NULL, rather than the intended assignment of ptr.
But the idea has merit; I'll see if I can come up with a variant that still evaluates ptr only once, by wrapping the type check in a sizeof hack.
I also get more warnings about casting from const to non-const, this can be avoided by making check_type const void *, but maybe these warnings indicate that something that shouldn't be freed is freed.
How many warnings? If it's only a handful, we should start investigating; if it's lots, then I'm not sure how much it buys us.
I just took a look at it and the first instances are mostly const char* used for allocated strings. I didn't spot a real error yet. But I'll leaf this one to you and finish my generator series instead :) Matthias
On Fri, Apr 22, 2011 at 07:06:18PM +0200, Matthias Bolte wrote:
I just took a look at it and the first instances are mostly const char* used for allocated strings. I didn't spot a real error yet.
Same here, looked at a dozen of these warnings, and just const char * being used for allocated strings, no error. It's probably better not to warn on this since it happens quite often and is harmless. Christophe
2011/4/22 Eric Blake <eblake@redhat.com>:
On 04/22/2011 10:11 AM, Matthias Bolte wrote:
--- daemon/remote.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 54fef64..1c98bba 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3681,7 +3681,7 @@ remoteDispatchListNetworks(struct qemud_server *server ATTRIBUTE_UNUSED, cleanup: if (rv < 0) { remoteDispatchError(rerr); - VIR_FREE(ret->names.names_len); + VIR_FREE(ret->names.names_val);
And to think I missed those in the huge patch. Oops.
ACK.
Thanks, pushed. Matthias
participants (3)
-
Christophe Fergeau -
Eric Blake -
Matthias Bolte