[PATCH 0/5] alloc: Don't mask use of uninitialized variables in VIR_FREE
Neither GCC nor clang report warnings if pointer passed to VIR_FREE is uninitialized. This is probably caused by our internal implementation which mangles the pointer to it. Fix all offenders and replace VIR_FREE's internals with g_clear_pointer. Peter Krempa (5): virNetworkPortDefSaveStatus: Fix potentially uninitialized 'path' by refactoring cleanup virLXCProcessSetupNamespaceName: Fix potential uninitialized free of 'path' cmdDomHostname: Fix uninitialized use of 'hostname' by refactoring cleanup testQemuMonitorJSONqemuMonitorJSONGetTargetArch: Fix uninitialized use of 'arch' VIR_ALLOC: Replace internals by g_clear_pointer src/conf/virnetworkportdef.c | 19 +++++++------------ src/libvirt_private.syms | 1 - src/lxc/lxc_process.c | 3 +-- src/util/viralloc.c | 21 ++------------------- src/util/viralloc.h | 7 +------ tests/qemumonitorjsontest.c | 14 +++++--------- tools/virsh-domain.c | 18 ++++++------------ 7 files changed, 22 insertions(+), 61 deletions(-) -- 2.24.1
Use 'g_autofree' to clean both 'path' and 'xml' which mandates initialization and get rid of the 'cleanup' label and 'ret variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/virnetworkportdef.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/conf/virnetworkportdef.c b/src/conf/virnetworkportdef.c index b58e2ccae0..a4cffea8b6 100644 --- a/src/conf/virnetworkportdef.c +++ b/src/conf/virnetworkportdef.c @@ -444,29 +444,24 @@ virNetworkPortDefSaveStatus(virNetworkPortDef *def, const char *dir) { char uuidstr[VIR_UUID_STRING_BUFLEN]; - char *path; - char *xml = NULL; - int ret = -1; + g_autofree char *path = NULL; + g_autofree char *xml = NULL; virUUIDFormat(def->uuid, uuidstr); if (virFileMakePath(dir) < 0) - goto cleanup; + return -1; if (!(path = virNetworkPortDefConfigFile(dir, uuidstr))) - goto cleanup; + return -1; if (!(xml = virNetworkPortDefFormat(def))) - goto cleanup; + return -1; if (virXMLSaveFile(path, uuidstr, "net-port-create", xml) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(xml); - VIR_FREE(path); - return ret; + return 0; } -- 2.24.1
'path' could be accessed uninitialized. Fix it by using g_autofree which also mandates initialization. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/lxc/lxc_process.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0277ba8b62..cd0026f78f 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -409,7 +409,7 @@ static int virLXCProcessSetupNamespaceName(virLXCDriverPtr driver, int fd = -1; virDomainObjPtr vm; virLXCDomainObjPrivatePtr priv; - char *path; + g_autofree char *path = NULL; vm = virDomainObjListFindByName(driver->domains, name); if (!vm) { @@ -436,7 +436,6 @@ static int virLXCProcessSetupNamespaceName(virLXCDriverPtr driver, } cleanup: - VIR_FREE(path); virDomainObjEndAPI(&vm); return fd; } -- 2.24.1
Use 'g_autoptr' which mandates initialization for 'hostname' and also for 'domain' to allow full refactor of the cleanup path. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 9d0f7d68d2..8591e483a5 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -11885,9 +11885,8 @@ VIR_ENUM_IMPL(virshDomainHostnameSource, static bool cmdDomHostname(vshControl *ctl, const vshCmd *cmd) { - char *hostname; - virDomainPtr dom; - bool ret = false; + g_autofree char *hostname = NULL; + g_autoptr(virshDomain) dom = NULL; const char *sourcestr = NULL; int flags = 0; /* Use default value. Drivers can have its own default. */ @@ -11895,14 +11894,14 @@ cmdDomHostname(vshControl *ctl, const vshCmd *cmd) return false; if (vshCommandOptStringReq(ctl, cmd, "source", &sourcestr) < 0) - goto error; + return false; if (sourcestr) { int source = virshDomainHostnameSourceTypeFromString(sourcestr); if (source < 0) { vshError(ctl, _("Unknown data source '%s'"), sourcestr); - goto error; + return false; } switch ((virshDomainHostnameSource) source) { @@ -11920,16 +11919,11 @@ cmdDomHostname(vshControl *ctl, const vshCmd *cmd) hostname = virDomainGetHostname(dom, flags); if (hostname == NULL) { vshError(ctl, "%s", _("failed to get hostname")); - goto error; + return false; } vshPrint(ctl, "%s\n", hostname); - ret = true; - - error: - VIR_FREE(hostname); - virshDomainFree(dom); - return ret; + return true; } /** -- 2.24.1
Refactor the cleanup control flow and use g_autofree for 'arch' so that it's mandated that it's initialized. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemumonitorjsontest.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index e9f95e317d..c7049bcdf0 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -2162,8 +2162,7 @@ testQemuMonitorJSONqemuMonitorJSONGetTargetArch(const void *opaque) { const testGenericData *data = opaque; virDomainXMLOptionPtr xmlopt = data->xmlopt; - int ret = -1; - char *arch; + g_autofree char *arch = NULL; g_autoptr(qemuMonitorTest) test = NULL; if (!(test = qemuMonitorTestNewSchema(xmlopt, data->schema))) @@ -2176,22 +2175,19 @@ testQemuMonitorJSONqemuMonitorJSONGetTargetArch(const void *opaque) " }," " \"id\": \"libvirt-21\"" "}") < 0) - goto cleanup; + return -1; if (!(arch = qemuMonitorJSONGetTargetArch(qemuMonitorTestGetMonitor(test)))) - goto cleanup; + return -1; if (STRNEQ(arch, "x86_64")) { virReportError(VIR_ERR_INTERNAL_ERROR, "Unexpected architecture %s, expecting x86_64", arch); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(arch); - return ret; + return 0; } static int -- 2.24.1
Our implementation masks GCC warnings of uninitialized use of the passed argument. After changing this I got a load of following warnings: src/conf/virnetworkportdef.c: In function 'virNetworkPortDefSaveStatus': /usr/include/glib-2.0/glib/gmem.h:136:8: error: 'path' may be used uninitialized in this function [-Werror=maybe-uninitialized] 136 | if (_p) \ | ^ src/conf/virnetworkportdef.c:447:11: note: 'path' was declared here 447 | char *path; | ^~~~ For the curious, g_clear_pointer is still safe for arguments with side-effect. Here's the post-processed output of trying to do a VIR_FREE(*(test2++)): do { typedef char _GStaticAssertCompileTimeAssertion_1[(sizeof *(&(*(test2++))) == sizeof (gpointer)) ? 1 : -1] __attribute__((__unused__)); __typeof__((&(*(test2++)))) _pp = (&(*(test2++))); __typeof__(*(&(*(test2++)))) _ptr = *_pp; *_pp = ((void *)0); if (_ptr) (g_free) (_ptr); } while (0) ; Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/viralloc.c | 21 ++------------------- src/util/viralloc.h | 7 +------ 3 files changed, 3 insertions(+), 26 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8563695c32..d5f6d7ec05 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1584,7 +1584,6 @@ virDeleteElementsN; virDispose; virDisposeString; virExpandN; -virFree; virInsertElementsN; virReallocN; virResizeN; diff --git a/src/util/viralloc.c b/src/util/viralloc.c index b8ca850764..e254416cdf 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -178,7 +178,8 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) if (virReallocN(ptrptr, size, *countptr -= toremove) < 0) abort(); } else { - virFree(ptrptr); + g_free(*((void **)ptrptr)); + *((void **)ptrptr) = NULL; *countptr = 0; } } @@ -333,24 +334,6 @@ int virAllocVar(void *ptrptr, } -/** - * virFree: - * @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) -{ - int save_errno = errno; - - g_free(*(void**)ptrptr); - *(void**)ptrptr = NULL; - errno = save_errno; -} - - /** * virDispose: * @ptrptr: pointer to pointer for address of memory to be sanitized and freed diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 1d42aeead1..833f85f62e 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -55,7 +55,6 @@ int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4); int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1); -void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countptr) ATTRIBUTE_NONNULL(1); @@ -417,11 +416,7 @@ void virDisposeString(char **strptr) * * This macro is safe to use on arguments with side effects. */ -/* The ternary ensures that ptr is a non-const pointer and not an - * integer type, all while evaluating ptr only once. This gives us - * extra compiler safety when compiling under gcc. - */ -#define VIR_FREE(ptr) virFree(1 ? (void *) &(ptr) : (ptr)) +#define VIR_FREE(ptr) g_clear_pointer(&(ptr), g_free) /** -- 2.24.1
On a Thursday in 2020, Peter Krempa wrote:
Neither GCC nor clang report warnings if pointer passed to VIR_FREE is uninitialized. This is probably caused by our internal implementation which mangles the pointer to it.
Fix all offenders and replace VIR_FREE's internals with g_clear_pointer.
Peter Krempa (5): virNetworkPortDefSaveStatus: Fix potentially uninitialized 'path' by refactoring cleanup virLXCProcessSetupNamespaceName: Fix potential uninitialized free of 'path' cmdDomHostname: Fix uninitialized use of 'hostname' by refactoring cleanup testQemuMonitorJSONqemuMonitorJSONGetTargetArch: Fix uninitialized use of 'arch' VIR_ALLOC: Replace internals by g_clear_pointer
src/conf/virnetworkportdef.c | 19 +++++++------------ src/libvirt_private.syms | 1 - src/lxc/lxc_process.c | 3 +-- src/util/viralloc.c | 21 ++------------------- src/util/viralloc.h | 7 +------ tests/qemumonitorjsontest.c | 14 +++++--------- tools/virsh-domain.c | 18 ++++++------------ 7 files changed, 22 insertions(+), 61 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa