11:11 a.m.
---
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
11:17 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:31 a.m.
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
11:55 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:04 p.m.
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
1:21 p.m.
New subject: [libvirt] [PATCH] util: warn when passing a non-pointer to VIR_FREE
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
1:25 p.m.
New subject: [libvirt] [PATCH] util: warn when passing a non-pointer to VIR_FREE
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
6:36 p.m.
New subject: [libvirt] [PATCH] util: warn when passing a non-pointer to VIR_FREE
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5:17 a.m.
New subject: [libvirt] [PATCH] util: warn when passing a non-pointer to VIR_FREE
2011/4/23 Eric Blake <eblake(a)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
11:38 a.m.
New subject: [libvirt] [PATCH] util: warn when passing a non-pointer to VIR_FREE
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(a)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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
3:03 p.m.
New subject: [libvirt] [PATCH] util: warn when passing a non-pointer to VIR_FREE
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
12:06 p.m.
2011/4/22 Eric Blake <eblake(a)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
12:17 p.m.
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
12:04 p.m.
2011/4/22 Eric Blake <eblake(a)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
4959
days inactive
4963
days old
13 comments
3 participants
participants (3)
-
Christophe Fergeau -
Eric Blake -
Matthias Bolte