In many places we define a variable as a 'const char *' when in fact we modify it just a few lines below. Or even free it. We should not do that. There's one exception though, in xenSessionFree() xenapi_utils.c. We are freeing the xen_session structure which is defined in xen/api/xen_common.h public header. The structure contains session_id which is type of 'const char *' when in fact it should have been just 'char *'. So I'm leaving this unmodified, just noticing the fact in comment. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/locking/lock_driver_lockd.c | 2 +- src/qemu/qemu_capabilities.c | 2 +- src/remote/remote_driver.c | 2 +- src/xenapi/xenapi_utils.c | 1 + tools/virsh-domain.c | 4 ++-- tools/wireshark/src/packet-libvirt.c | 6 +++--- tools/wireshark/src/packet-libvirt.h | 4 ++-- 7 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 1ca7772..367d0ce 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -243,7 +243,7 @@ static virNetClientPtr virLockManagerLockDaemonConnectionNew(bool privileged, { virNetClientPtr client = NULL; char *lockdpath; - const char *daemonPath = NULL; + char *daemonPath = NULL; *prog = NULL; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 37e0588..8271e28 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2652,7 +2652,7 @@ static int virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) { virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *xml = NULL; + char *xml = NULL; int ret = -1; size_t i; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 9d8120f..9a1d78f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -582,7 +582,7 @@ doRemoteOpen(virConnectPtr conn, trans_tcp, } transport; #ifndef WIN32 - const char *daemonPath = NULL; + char *daemonPath = NULL; #endif /* We handle *ALL* URIs here. The caller has rejected any diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index 8b28914..a80d136 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -49,6 +49,7 @@ xenSessionFree(xen_session *session) VIR_FREE(session->error_description[i]); VIR_FREE(session->error_description); } + /* The session_id member is type of 'const char *'. Sigh. */ VIR_FREE(session->session_id); VIR_FREE(session); } diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5a17aff..ba47258 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -10349,8 +10349,8 @@ vshPrepareDiskXML(xmlNodePtr disk_node, int type) { xmlNodePtr cur = NULL; - const char *disk_type = NULL; - const char *device_type = NULL; + char *disk_type = NULL; + char *device_type = NULL; xmlNodePtr new_node = NULL; char *ret = NULL; diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c index 0fe5f03..5c3c369 100644 --- a/tools/wireshark/src/packet-libvirt.c +++ b/tools/wireshark/src/packet-libvirt.c @@ -105,7 +105,7 @@ dissect_xdr_string(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, } } -static gchar * +static const gchar * format_xdr_bytes(guint8 *bytes, guint32 length) { gchar *buf; @@ -206,7 +206,7 @@ dissect_xdr_iterable(tvbuff_t *tvb, proto_item *ti, XDR *xdrs, gint ett, int rhf static gboolean dissect_xdr_vector(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 size, vir_xdr_dissector_t dissect) + int rhf, const gchar *rtype, guint32 size, vir_xdr_dissector_t dissect) { goffset start; proto_item *ti; @@ -219,7 +219,7 @@ dissect_xdr_vector(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, static gboolean dissect_xdr_array(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dissect) + int rhf, const gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dissect) { goffset start; proto_item *ti; diff --git a/tools/wireshark/src/packet-libvirt.h b/tools/wireshark/src/packet-libvirt.h index af54407..5f99fdf 100644 --- a/tools/wireshark/src/packet-libvirt.h +++ b/tools/wireshark/src/packet-libvirt.h @@ -105,9 +105,9 @@ static gboolean dissect_xdr_bytes(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, in static gboolean dissect_xdr_pointer(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, vir_xdr_dissector_t dp); static gboolean dissect_xdr_vector(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 size, vir_xdr_dissector_t dp); + int rhf, const gchar *rtype, guint32 size, vir_xdr_dissector_t dp); static gboolean dissect_xdr_array(tvbuff_t *tvb, proto_tree *tree, XDR *xdrs, int hf, gint ett, - int rhf, gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dp); + int rhf, const gchar *rtype, guint32 maxlen, vir_xdr_dissector_t dp); # include "libvirt/protocol.h" -- 1.8.5.5

Since we've corrected all the callers that passed a const pointer to VIR_FREE() we may drop the typecasting horribility and let the macro be a simple wrapper over the virFree() function. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/viralloc.h | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 7125e67..71b4a45 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -547,20 +547,7 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * * This macro is safe to use on arguments with side effects. */ -# if !STATIC_ANALYSIS -/* The ternary ensures that ptr is a pointer and not an integer type, - * while evaluating ptr only once. This gives us extra compiler - * safety when compiling under gcc. 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))) -# else -/* The Coverity static analyzer considers the else path of the "?:" and - * flags the VIR_FREE() of the address of the address of memory as a - * RESOURCE_LEAK resulting in numerous false positives (eg, VIR_FREE(&ptr)) - */ -# define VIR_FREE(ptr) virFree((void *) &(ptr)) -# endif +# define VIR_FREE(ptr) virFree(&(ptr)) void virAllocTestInit(void); int virAllocTestCount(void); -- 1.8.5.5

Up to now it's possible to do something like this: const char *ptr; ptr = strdup("my example string"); VIR_FREE(ptr); The problem is, const char * pointers should not be modified (and freeing them is kind of modification). We should avoid this. A little trick is used: assigning a const pointer into 'void *' triggers compiler warning about discarding 'const' qualifier from pointer. So the virFree() function gains new dummy argument, that is not touched anyhow, just fulfills the const correctness check duty. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/viralloc.c | 6 ++++-- src/util/viralloc.h | 20 ++++++++++++++++---- src/xenapi/xenapi_utils.c | 2 +- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/util/viralloc.c b/src/util/viralloc.c index be9f0fe..0134e67 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -372,7 +372,7 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) ignore_value(virReallocN(ptrptr, size, *countptr -= toremove, false, 0, NULL, NULL, 0)); else { - virFree(ptrptr); + virFree(NULL, ptrptr); *countptr = 0; } } @@ -569,13 +569,15 @@ int virAllocVar(void *ptrptr, /** * virFree: + * @ptr: dummy pointer to check const correctness * @ptrptr: pointer to pointer for address of memory to be freed * * Release the chunk of memory in the pointer pointed to by * the 'ptrptr' variable. After release, 'ptrptr' will be * updated to point to NULL. */ -void virFree(void *ptrptr) +void virFree(void *ptr ATTRIBUTE_UNUSED, + void *ptrptr) { int save_errno = errno; diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 71b4a45..8fbe56f 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -76,7 +76,7 @@ int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t co bool report, int domcode, const char *filename, const char *funcname, size_t linenr) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); -void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); +void virFree(void *ptr, void *ptrptr) ATTRIBUTE_NONNULL(2); /** * VIR_ALLOC: @@ -543,11 +543,23 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * @ptr: pointer holding address to be freed * * Free the memory stored in 'ptr' and update to point - * to NULL. + * to NULL. Moreover, this macro has a side effect in + * form of evaluating passed argument multiple times. + * But advantage is, it checks the cont correctness + * (that you are not freeing a const pointer). + */ +# define VIR_FREE(ptr) virFree(ptr, &(ptr)) + +/** + * VIR_FREE_BROKEN: + * @ptr: pointer holding address to be freed * - * This macro is safe to use on arguments with side effects. + * Twin macro of VIR_FREE. While it does evaluate + * argument only once, it does not check const + * correctness and therefore you want to use it if and + * only if necessary. */ -# define VIR_FREE(ptr) virFree(&(ptr)) +# define VIR_FREE_BROKEN(ptr) virFree(NULL, &(ptr)) void virAllocTestInit(void); int virAllocTestCount(void); diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index a80d136..db555d2 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -50,7 +50,7 @@ xenSessionFree(xen_session *session) VIR_FREE(session->error_description); } /* The session_id member is type of 'const char *'. Sigh. */ - VIR_FREE(session->session_id); + VIR_FREE_BROKEN(session->session_id); VIR_FREE(session); } -- 1.8.5.5

On 15.07.2014 15:27, Martin Kletzander wrote: That won't work. Consider: char *tmp; VIR_FREE(tmp); which in turn is equal to: virFree(&tmp); so the &tmp is type of 'char **' while virFree() would expect 'void **' which confuses compiler. Michal

On 07/15/2014 07:46 AM, Eric Blake wrote: I didn't even need to resort to sizeof(). Here's v2, which replaces 2, 3, and 4 of this series: https://www.redhat.com/archives/libvir-list/2014-July/msg00760.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org