[libvirt PATCH v2 00/10] Random bits found by clang-tidy
clang-tidy is a static code analysis tool under the llvm umbrella. It is primarily meant to be used on C++ code bases, but some of the checks it provides also apply to C. The findings vary in severity and contain pseudo-false-positives, i.e. clang-tidy is flagging potential execution flows that could happen in theory but are virtually impossible in real life: In function `virGetUnprivSGIOSysfsPath`, variables `maj` and `min` would be read unintialized if `stat()` failed and set `errno` to a negative value, to name just one example. The main source of false positive findings is the lack of support for `__attribute__((cleanup))` in clang-tidy, which is heavily used in libvirt through glib's `g_autofree` and `g_auto()` macros: #include <stdlib.h> void freeptr(int** p) { if (*p) free(*p); } int main() { __attribute__((cleanup(freeptr))) int *ptr = NULL; ptr = calloc(sizeof(int), 1); return 0; /* flagged as memory leak of `ptr` */ } This sadly renders clang-tidy's analysis of dynamic memory useless, hiding all real issues that it could otherwise find. Meson provides excellent integration for clang-tidy (a "clang-tidy" target is automatically generated if a ".clang-tidy" configuration file is present in the project's root directory). The amount of false-positives (many of which present in a group of checks that cannot be disabled), random segfaults in memory constraint environments such as the containers in the CI, and the slow analysis (triggering time-outs in the CI), make this tool unfit for inclusion in libvirt's GitLab CI though. The patches in this series are the result of fixing some of the issues reported by running CC=clang meson build ninja -C build # generate sources and header files ninja -C build clang-tidy with the following `.clang-tidy` configuration file: --- Checks: > *, -abseil-*, -android-*, -boost-*, -cppcoreguidelines-*, -fuchsia-*, -google-*, -hicpp-*, -llvm-*, -modernize-*, -mpi-, -objc-, -openmp-, -zircon-*, -readability-braces-around-statements, -readability-magic-numbers WarningsAsErrors: '*' HeaderFilterRegex: '' FormatStyle: none ... V1: https://www.redhat.com/archives/libvir-list/2021-January/msg01152.html Changes since V1: * Expanded the justification for the "Replace bzero() with memset()" patch. * Rewrote "udevProcessCCW: Initialize variable". * Rewrote "tests: Prevent mallo with size 0". * Dropped "vircommand: Remove NULL check in virCommandAddArg". Note that this series now depends on the rewrite of tests/commandhelper.c, which can be found here: https://www.redhat.com/archives/libvir-list/2021-February/msg00034.html Regards, Tim Tim Wiederhake (10): virfile: Remove redundant #ifndef xen: Fix indentation in xenParseXLSpice qemu_tpm: Fix indentation in qemuTPMEmulatorBuildCommand virsh-domain: Fix error handling of pthread_sigmask Replace bzero() with memset() udevProcessCCW: Initialize variable virhostuptime: Fix rounding in uptime calculation tests: Prevent malloc with size 0 vircryptotest: Directly assign string to avoid memcpy vircommand: Simplify virCommandAddArg src/libxl/xen_xl.c | 5 ++--- src/node_device/node_device_udev.c | 2 +- src/qemu/qemu_tpm.c | 8 +++++--- src/util/virarptable.c | 2 +- src/util/vircommand.c | 7 +------ src/util/virfile.c | 4 ++-- src/util/virhostuptime.c | 4 +++- tests/commandhelper.c | 3 +++ tests/vircryptotest.c | 5 +---- tests/virpcimock.c | 2 +- tools/virsh-domain.c | 8 ++++---- 11 files changed, 24 insertions(+), 26 deletions(-) -- 2.26.2
This section is guarded by "#ifndef WIN32" in line 2109--2808. Found by clang-tidy's "readability-redundant-preprocessor" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 609cafdd34..5201b7fb07 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2632,7 +2632,7 @@ virDirCreateNoFork(const char *path, virReportSystemError(errno, _("stat of '%s' failed"), path); goto error; } -# ifndef WIN32 + if (((uid != (uid_t) -1 && st.st_uid != uid) || (gid != (gid_t) -1 && st.st_gid != gid)) && (chown(path, uid, gid) < 0)) { @@ -2641,7 +2641,7 @@ virDirCreateNoFork(const char *path, path, (unsigned int) uid, (unsigned int) gid); goto error; } -# endif /* !WIN32 */ + if (mode != (mode_t) -1 && chmod(path, mode) < 0) { ret = -errno; virReportSystemError(errno, -- 2.26.2
This was found by clang-tidy's "readability-misleading-indentation" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/xen_xl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libxl/xen_xl.c b/src/libxl/xen_xl.c index 621ee63a99..6dcba43fe0 100644 --- a/src/libxl/xen_xl.c +++ b/src/libxl/xen_xl.c @@ -359,9 +359,8 @@ xenParseXLSpice(virConfPtr conf, virDomainDefPtr def) graphics->data.spice.port = (int)port; - if (!graphics->data.spice.tlsPort && - !graphics->data.spice.port) - graphics->data.spice.autoport = 1; + if (!graphics->data.spice.tlsPort && !graphics->data.spice.port) + graphics->data.spice.autoport = 1; if (xenConfigGetBool(conf, "spicedisable_ticketing", &val, 0) < 0) goto cleanup; -- 2.26.2
This was found by clang-tidy's "readability-misleading-indentation" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_tpm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 532e0912bd..f94cad8230 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -600,9 +600,11 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDefPtr tpm, } pwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, cmd); - if (pwdfile_fd) - migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, - cmd); + if (pwdfile_fd) { + migpwdfile_fd = qemuTPMSetupEncryption(tpm->data.emulator.secretuuid, + cmd); + } + if (pwdfile_fd < 0 || migpwdfile_fd < 0) goto error; -- 2.26.2
pthread_sigmask() returns 0 on success and "a non-zero value on failure", but not neccessarily a negative one. Found by clang-tidy's "bugprone-posix-return" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 2bb136333f..298c3a6587 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -4226,7 +4226,7 @@ doSave(void *opaque) sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); - if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) != 0) goto out_sig; #endif /* !WIN32 */ @@ -4756,7 +4756,7 @@ doManagedsave(void *opaque) sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); - if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) != 0) goto out_sig; #endif /* !WIN32 */ @@ -5438,7 +5438,7 @@ doDump(void *opaque) sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); - if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) != 0) goto out_sig; #endif /* !WIN32 */ @@ -10732,7 +10732,7 @@ doMigrate(void *opaque) sigemptyset(&sigmask); sigaddset(&sigmask, SIGINT); - if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) < 0) + if (pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask) != 0) goto out_sig; #endif /* !WIN32 */ -- 2.26.2
This was found by clang-tidy's "clang-analyzer-security.insecureAPI.bzero" check. bzero is marked as deprecated ("LEGACY") in POSIX.1-2001 and removed in POSIX.1-2008. Besides its deprecation, bzero can be unsafe to use under certain circumstances, e.g. when used to zero-out memory containing secrects. These calls can be optimized away by the compiler, if it concludes no further access happens to the memory, thus leaving the secrets still in memory. Hence its classification as "insecureAPI". Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virarptable.c | 2 +- tests/virpcimock.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virarptable.c b/src/util/virarptable.c index d62de5e3dd..dac3486470 100644 --- a/src/util/virarptable.c +++ b/src/util/virarptable.c @@ -120,7 +120,7 @@ virArpTableGet(void) table->n = num + 1; addr = RTA_DATA(tb[NDA_DST]); - bzero(&virAddr, sizeof(virAddr)); + memset(&virAddr, 0, sizeof(virAddr)); virAddr.len = sizeof(virAddr.data.inet4); virAddr.data.inet4.sin_family = AF_INET; virAddr.data.inet4.sin_addr = *(struct in_addr *)addr; diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 4aa96cae08..f6280fc8b5 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -233,7 +233,7 @@ pci_read_file(const char *path, if ((fd = real_open(newpath, O_RDWR)) < 0) goto cleanup; - bzero(buf, buf_size); + memset(buf, 0, buf_size); if (saferead(fd, buf, buf_size - 1) < 0) { STDERR("Unable to read from %s", newpath); goto cleanup; -- 2.26.2
On Mon, Feb 01, 2021 at 13:42:02 +0100, Tim Wiederhake wrote:
This was found by clang-tidy's "clang-analyzer-security.insecureAPI.bzero" check.
bzero is marked as deprecated ("LEGACY") in POSIX.1-2001 and removed in POSIX.1-2008.
Besides its deprecation, bzero can be unsafe to use under certain circumstances, e.g. when used to zero-out memory containing secrects. These calls can be optimized away by the compiler, if it concludes no further access happens to the memory, thus leaving the secrets still in memory. Hence its classification as "insecureAPI".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virarptable.c | 2 +- tests/virpcimock.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
`udevGetIntSysfsAttr` does not necessarily write to the third parameter, even when it returns 0. This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index fceb135aa5..09048fb23f 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1100,7 +1100,7 @@ static int udevProcessCCW(struct udev_device *device, virNodeDeviceDefPtr def) { - int online; + int online = 0; /* process only online devices to keep the list sane */ if (udevGetIntSysfsAttr(device, "online", &online, 0) < 0 || online != 1) -- 2.26.2
On Mon, Feb 01, 2021 at 13:42:03 +0100, Tim Wiederhake wrote:
`udevGetIntSysfsAttr` does not necessarily write to the third parameter, even when it returns 0.
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
"f + 0.5" does not round correctly for values very close to ".5" for every integer multiple, e.g. "0.499999975". Found by clang-tidy's "bugprone-incorrect-roundings" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virhostuptime.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virhostuptime.c b/src/util/virhostuptime.c index 696a38fbb5..7508a5a9b6 100644 --- a/src/util/virhostuptime.c +++ b/src/util/virhostuptime.c @@ -32,6 +32,8 @@ #include "virtime.h" #include "virthread.h" +#include <math.h> + #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("util.virhostuptime"); @@ -76,7 +78,7 @@ virHostGetBootTimeProcfs(unsigned long long *btime) return -EINVAL; } - *btime = now / 1000 - up + 0.5; + *btime = llround(now / 1000 - up); return 0; } -- 2.26.2
Found by clang-tidy's "clang-analyzer-optin.portability.UnixAPI" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index bf6a5baa40..b3c65ab3cc 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -156,6 +156,9 @@ static int printEnvironment(FILE *log) for (length = 0; environ[length]; length++) { } + if (length == 0) + return 0; + if (!(newenv = malloc(sizeof(*newenv) * length))) return -1; -- 2.26.2
On Mon, Feb 01, 2021 at 13:42:05 +0100, Tim Wiederhake wrote:
Found by clang-tidy's "clang-analyzer-optin.portability.UnixAPI" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/commandhelper.c | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Found by clang-tidy's "bugprone-not-null-terminated-result" check. clang-tidy's finding is a false positive in this case, as the memset call guarantees null termination. The assignment can be simplified though, and this happens to silence the warning. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- tests/vircryptotest.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index 90140077cf..2c3d4bfe75 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -122,7 +122,7 @@ static int mymain(void) { int ret = 0; - uint8_t secretdata[8]; + uint8_t secretdata[8] = "letmein"; uint8_t expected_ciphertext[16] = {0x48, 0x8e, 0x9, 0xb9, 0x6a, 0xa6, 0x24, 0x5f, 0x1b, 0x8c, 0x3f, 0x48, @@ -166,9 +166,6 @@ mymain(void) ret = -1; \ } while (0) - memset(&secretdata, 0, 8); - memcpy(&secretdata, "letmein", 7); - VIR_CRYPTO_ENCRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes265cbc", secretdata, 7, expected_ciphertext, 16); -- 2.26.2
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> Reviewed-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircommand.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c3a98bbeac..b1a26f68aa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1520,8 +1520,6 @@ virCommandAddEnvXDG(virCommandPtr cmd, const char *baseDir) void virCommandAddArg(virCommandPtr cmd, const char *val) { - char *arg; - if (!cmd || cmd->has_error) return; @@ -1530,16 +1528,13 @@ virCommandAddArg(virCommandPtr cmd, const char *val) return; } - arg = g_strdup(val); - /* Arg plus trailing NULL. */ if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { - VIR_FREE(arg); cmd->has_error = ENOMEM; return; } - cmd->args[cmd->nargs++] = arg; + cmd->args[cmd->nargs++] = g_strdup(val); } -- 2.26.2
participants (2)
-
Peter Krempa -
Tim Wiederhake