[libvirt PATCH 0/8] Some cppcheck-inspired fixes
At least I found a memleak. Ján Tomko (8): vz: remove redundant NULL pointer check Remove redundant conditions util: command: do not return after abort util: virhostmem: do not use scanf without field limits util: sync variable names between header and C files virsh: completer: use signed variable for XPathNodeSet errors tools: remove unread variables tools: virt-admin: do not leak daemon-log settings src/hypervisor/virhostdev.c | 2 +- src/libxl/xen_xl.c | 2 +- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_process.c | 6 +++--- src/rpc/virnetsocket.c | 2 +- src/util/vircommand.c | 1 - src/util/virhostmem.c | 6 ++++-- src/util/virlease.c | 2 +- src/util/virresctrl.h | 2 +- src/util/virsocketaddr.h | 8 ++++---- src/vbox/vbox_common.c | 2 +- src/vz/vz_sdk.c | 8 ++++---- tools/virsh-completer-host.c | 4 ++-- tools/virsh-domain.c | 7 +++---- tools/virt-admin.c | 21 +++++++++------------ 15 files changed, 36 insertions(+), 40 deletions(-) -- 2.26.2
The 'dom' pointer is already dereferenced earlier. src/vz/vz_sdk.c:249:24: warning: Either the condition 'if(dom)' is redundant or there is possible null pointer dereference: dom. [nullPointerRedundantCheck] Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/vz/vz_sdk.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index b5e69b385f..bb1ef0ea47 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -256,11 +256,11 @@ waitDomainJobHelper(PRL_HANDLE job, virDomainObjPtr dom, unsigned int timeout, } pdom->job.sdkJob = job; - if (dom) - virObjectUnlock(dom); + + virObjectUnlock(dom); ret = waitJobHelper(job, timeout, filename, funcname, linenr); - if (dom) - virObjectLock(dom); + virObjectLock(dom); + pdom->job.sdkJob = NULL; return ret; -- 2.26.2
All of these have been checked earlier. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/hypervisor/virhostdev.c | 2 +- src/libxl/xen_xl.c | 2 +- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_process.c | 6 +++--- src/rpc/virnetsocket.c | 2 +- src/util/virlease.c | 2 +- src/vbox/vbox_common.c | 2 +- 7 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index 9017cc3be8..69102b8ebf 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -1425,7 +1425,7 @@ virHostdevFindUSBDevice(virDomainHostdevDefPtr hostdev, usbsrc->bus, usbsrc->device, bus, device); } - } else if (!vendor && bus) { + } else if (bus) { if (virUSBDeviceFindByBus(bus, device, NULL, mandatory, usb) < 0) return -1; } diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index f9dc18ab18..e83a019bcc 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -450,7 +450,7 @@ xenParseXLVnuma(virConfPtr conf, unsigned long long kbsize = 0; /* Is there a sublist (vnode)? */ - if (list && list->type == VIR_CONF_LIST) { + if (list->type == VIR_CONF_LIST) { vnode = list->list; while (vnode && vnode->type == VIR_CONF_STRING) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 26912334d2..b902c1aba1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4711,8 +4711,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, qemuDomainObjEnterMonitor(driver, vm); - if (rc == 0 && - qemuMonitorDelObject(priv->mon, objAlias, true) < 0) + if (qemuMonitorDelObject(priv->mon, objAlias, true) < 0) rc = -1; if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ec6ca14bbd..e99d9f76fe 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6888,11 +6888,11 @@ qemuProcessLaunch(virConnectPtr conn, goto cleanup; VIR_DEBUG("Handshake complete, child running"); - if (rv == -1) /* The VM failed to start; tear filters before taps */ + if (rv == -1) { + /* The VM failed to start; tear filters before taps */ virDomainConfVMNWFilterTeardown(vm); - - if (rv == -1) /* The VM failed to start */ goto cleanup; + } if (qemuDomainObjStartWorker(vm) < 0) goto cleanup; diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 5a7f501923..977d2875fe 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -687,7 +687,7 @@ int virNetSocketNewConnectUNIX(const char *path, if (spawnDaemon) { g_autofree char *binname = NULL; - if (spawnDaemon && !binary) { + if (!binary) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Auto-spawn of daemon requested, " "but no binary specified")); diff --git a/src/util/virlease.c b/src/util/virlease.c index 1cb9540d80..aeb605862f 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -244,7 +244,7 @@ virLeaseNew(virJSONValuePtr *lease_ret, return -1; if (ip && virJSONValueObjectAppendString(lease_new, "ip-address", ip) < 0) return -1; - if (mac && virJSONValueObjectAppendString(lease_new, "mac-address", mac) < 0) + if (virJSONValueObjectAppendString(lease_new, "mac-address", mac) < 0) return -1; if (hostname && virJSONValueObjectAppendString(lease_new, "hostname", hostname) < 0) return -1; diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 15f8eb074a..bdbb929ad6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -1201,7 +1201,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (disk->src->readonly) { gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable); VIR_DEBUG("Setting hard disk to immutable"); - } else if (!disk->src->readonly) { + } else { gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal); VIR_DEBUG("Setting hard disk type to normal"); } -- 2.26.2
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/vircommand.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5ce69ef241..76f7eb9a3d 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1509,7 +1509,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val) if (val == NULL) { cmd->has_error = EINVAL; abort(); - return; } arg = g_strdup(val); -- 2.26.2
We use an array of size VIR_NODE_MEMORY_STATS_FIELD_LENGTH to store the string read from sysfs, but pass unbound "%s" to sscanf. Make the array larger by one and simply stringify that constant as the field width specifier. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virhostmem.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c index 9097716f54..2f60e2a250 100644 --- a/src/util/virhostmem.c +++ b/src/util/virhostmem.c @@ -148,7 +148,7 @@ virHostMemGetStatsLinux(FILE *meminfo, int found = 0; int nr_param; char line[1024]; - char meminfo_hdr[VIR_NODE_MEMORY_STATS_FIELD_LENGTH]; + char meminfo_hdr[VIR_NODE_MEMORY_STATS_FIELD_LENGTH + 1]; unsigned long val; struct field_conv { const char *meminfo_hdr; /* meminfo header */ @@ -207,8 +207,10 @@ virHostMemGetStatsLinux(FILE *meminfo, buf = p; } - if (sscanf(buf, "%s %lu kB", meminfo_hdr, &val) < 2) +# define MEM_MAX_LEN G_STRINGIFY(VIR_NODE_MEMORY_STATS_FIELD_LENGTH) + if (sscanf(buf, "%" MEM_MAX_LEN "s %lu kB", meminfo_hdr, &val) < 2) continue; +# undef MEM_MAX_LEN for (j = 0; field_conv[j].meminfo_hdr != NULL; j++) { struct field_conv *convp = &field_conv[j]; -- 2.26.2
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virresctrl.h | 2 +- src/util/virsocketaddr.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virresctrl.h b/src/util/virresctrl.h index dcd9ca2bb2..2f84401dfb 100644 --- a/src/util/virresctrl.h +++ b/src/util/virresctrl.h @@ -151,7 +151,7 @@ virResctrlAllocSetMemoryBandwidth(virResctrlAllocPtr alloc, unsigned int memory_bandwidth); int -virResctrlAllocForeachMemory(virResctrlAllocPtr resctrl, +virResctrlAllocForeachMemory(virResctrlAllocPtr alloc, virResctrlAllocForeachMemoryCallback cb, void *opaque); diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index d06e751f73..6c08f8b35c 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -83,10 +83,10 @@ int virSocketAddrParseIPv6(virSocketAddrPtr addr, int virSocketAddrResolveService(const char *service); -void virSocketAddrSetIPv4AddrNetOrder(virSocketAddrPtr s, uint32_t addr); -void virSocketAddrSetIPv4Addr(virSocketAddrPtr s, uint32_t addr); -void virSocketAddrSetIPv6AddrNetOrder(virSocketAddrPtr s, uint32_t addr[4]); -void virSocketAddrSetIPv6Addr(virSocketAddrPtr s, uint32_t addr[4]); +void virSocketAddrSetIPv4AddrNetOrder(virSocketAddrPtr addr, uint32_t val); +void virSocketAddrSetIPv4Addr(virSocketAddrPtr addr, uint32_t val); +void virSocketAddrSetIPv6AddrNetOrder(virSocketAddrPtr addr, uint32_t val[4]); +void virSocketAddrSetIPv6Addr(virSocketAddrPtr addr, uint32_t val[4]); char *virSocketAddrFormat(const virSocketAddr *addr); char *virSocketAddrFormatFull(const virSocketAddr *addr, -- 2.26.2
Although virXPathNodeSet is unlikely to return -1, we should check for it properly or not at all. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-completer-host.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c index 35e6bba661..8893888ec2 100644 --- a/tools/virsh-completer-host.c +++ b/tools/virsh-completer-host.c @@ -55,7 +55,7 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, { g_autoptr(xmlXPathContext) ctxt = NULL; virshControlPtr priv = ctl->privData; - unsigned int npages = 0; + int npages = 0; g_autofree xmlNodePtr *pages = NULL; g_autoptr(xmlDoc) doc = NULL; size_t i = 0; @@ -106,7 +106,7 @@ virshCellnoCompleter(vshControl *ctl, { g_autoptr(xmlXPathContext) ctxt = NULL; virshControlPtr priv = ctl->privData; - unsigned int ncells = 0; + int ncells = 0; g_autofree xmlNodePtr *cells = NULL; g_autoptr(xmlDoc) doc = NULL; size_t i = 0; -- 2.26.2
Some of libvirt APIs return the number of elements, but we don't need them, only whether the API failed or not. Delete the redundant variables. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virsh-domain.c | 7 +++---- tools/virt-admin.c | 9 +++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index aaf3b9a6a5..97d6427638 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6886,7 +6886,6 @@ virshVcpuinfoInactive(vshControl *ctl, { g_autofree unsigned char *cpumaps = NULL; size_t cpumaplen; - int ncpus; g_autoptr(virBitmap) vcpus = NULL; ssize_t nextvcpu = -1; bool first = true; @@ -6897,9 +6896,9 @@ virshVcpuinfoInactive(vshControl *ctl, cpumaplen = VIR_CPU_MAPLEN(maxcpu); cpumaps = vshMalloc(ctl, virBitmapSize(vcpus) * cpumaplen); - if ((ncpus = virDomainGetVcpuPinInfo(dom, virBitmapSize(vcpus), - cpumaps, cpumaplen, - VIR_DOMAIN_AFFECT_CONFIG)) < 0) + if (virDomainGetVcpuPinInfo(dom, virBitmapSize(vcpus), + cpumaps, cpumaplen, + VIR_DOMAIN_AFFECT_CONFIG) < 0) return false; while ((nextvcpu = virBitmapNextSetBit(vcpus, nextvcpu)) >= 0) { diff --git a/tools/virt-admin.c b/tools/virt-admin.c index fef0332a0d..4856758b4e 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1040,7 +1040,6 @@ static const vshCmdOptDef opts_daemon_log_filters[] = { static bool cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) { - int nfilters; char *filters = NULL; vshAdmControlPtr priv = ctl->privData; @@ -1052,8 +1051,8 @@ cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) return false; } } else { - if ((nfilters = virAdmConnectGetLoggingFilters(priv->conn, - &filters, 0)) < 0) { + if (virAdmConnectGetLoggingFilters(priv->conn, + &filters, 0) < 0) { vshError(ctl, _("Unable to get daemon logging filters information")); return false; } @@ -1094,7 +1093,6 @@ static const vshCmdOptDef opts_daemon_log_outputs[] = { static bool cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) { - int noutputs; char *outputs = NULL; vshAdmControlPtr priv = ctl->privData; @@ -1106,8 +1104,7 @@ cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) return false; } } else { - if ((noutputs = virAdmConnectGetLoggingOutputs(priv->conn, - &outputs, 0)) < 0) { + if (virAdmConnectGetLoggingOutputs(priv->conn, &outputs, 0) < 0) { vshError(ctl, _("Unable to get daemon logging outputs information")); return false; } -- 2.26.2
The commands daemon-log-filters and daemon-log-outputs are used both for getting and setting the variables. But the getter receives an allocated string, which we do not free. Use separate variables for the getter and the setter to get rid of the memory leak and to stop casting away the const. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- tools/virt-admin.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/virt-admin.c b/tools/virt-admin.c index 4856758b4e..07d5f49825 100644 --- a/tools/virt-admin.c +++ b/tools/virt-admin.c @@ -1040,17 +1040,17 @@ static const vshCmdOptDef opts_daemon_log_filters[] = { static bool cmdDaemonLogFilters(vshControl *ctl, const vshCmd *cmd) { - char *filters = NULL; vshAdmControlPtr priv = ctl->privData; if (vshCommandOptBool(cmd, "filters")) { - if ((vshCommandOptStringReq(ctl, cmd, "filters", - (const char **) &filters) < 0 || + const char *filters = NULL; + if ((vshCommandOptStringReq(ctl, cmd, "filters", &filters) < 0 || virAdmConnectSetLoggingFilters(priv->conn, filters, 0) < 0)) { vshError(ctl, _("Unable to change daemon logging settings")); return false; } } else { + g_autofree char *filters = NULL; if (virAdmConnectGetLoggingFilters(priv->conn, &filters, 0) < 0) { vshError(ctl, _("Unable to get daemon logging filters information")); @@ -1093,17 +1093,17 @@ static const vshCmdOptDef opts_daemon_log_outputs[] = { static bool cmdDaemonLogOutputs(vshControl *ctl, const vshCmd *cmd) { - char *outputs = NULL; vshAdmControlPtr priv = ctl->privData; if (vshCommandOptBool(cmd, "outputs")) { - if ((vshCommandOptStringReq(ctl, cmd, "outputs", - (const char **) &outputs) < 0 || + const char *outputs = NULL; + if ((vshCommandOptStringReq(ctl, cmd, "outputs", &outputs) < 0 || virAdmConnectSetLoggingOutputs(priv->conn, outputs, 0) < 0)) { vshError(ctl, _("Unable to change daemon logging settings")); return false; } } else { + g_autofree char *outputs = NULL; if (virAdmConnectGetLoggingOutputs(priv->conn, &outputs, 0) < 0) { vshError(ctl, _("Unable to get daemon logging outputs information")); return false; -- 2.26.2
On Mon, Aug 03, 2020 at 12:11:23AM +0200, Ján Tomko wrote:
At least I found a memleak.
Ján Tomko (8): vz: remove redundant NULL pointer check Remove redundant conditions util: command: do not return after abort util: virhostmem: do not use scanf without field limits util: sync variable names between header and C files virsh: completer: use signed variable for XPathNodeSet errors tools: remove unread variables tools: virt-admin: do not leak daemon-log settings
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
src/hypervisor/virhostdev.c | 2 +- src/libxl/xen_xl.c | 2 +- src/qemu/qemu_hotplug.c | 3 +-- src/qemu/qemu_process.c | 6 +++--- src/rpc/virnetsocket.c | 2 +- src/util/vircommand.c | 1 - src/util/virhostmem.c | 6 ++++-- src/util/virlease.c | 2 +- src/util/virresctrl.h | 2 +- src/util/virsocketaddr.h | 8 ++++---- src/vbox/vbox_common.c | 2 +- src/vz/vz_sdk.c | 8 ++++---- tools/virsh-completer-host.c | 4 ++-- tools/virsh-domain.c | 7 +++---- tools/virt-admin.c | 21 +++++++++------------ 15 files changed, 36 insertions(+), 40 deletions(-)
-- 2.26.2
participants (2)
-
Ján Tomko -
Martin Kletzander