[libvirt PATCH 00/11] 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 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 ... Regards, Tim Tim Wiederhake (11): 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() udevGetIntSysfsAttr: Return -1 for missing attributes virhostuptime: Fix rounding in uptime calculation tests: Prevent malloc with size 0 vircryptotest: Directly assign string to avoid memcpy vircommand: Remove NULL check in virCommandAddArg vircommand: Simplify virCommandAddArg src/libxl/xen_xl.c | 5 ++--- src/node_device/node_device_udev.c | 5 ++++- src/qemu/qemu_tpm.c | 8 +++++--- src/util/virarptable.c | 2 +- src/util/vircommand.c | 12 +----------- src/util/virfile.c | 4 ++-- src/util/virhostuptime.c | 4 +++- tests/commandhelper.c | 2 +- tests/vircryptotest.c | 5 +---- tests/virpcimock.c | 2 +- tools/virsh-domain.c | 8 ++++---- 11 files changed, 25 insertions(+), 32 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> --- 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> --- 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> --- 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> --- 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. 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 Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
This was found by clang-tidy's "clang-analyzer-security.insecureAPI.bzero" check.
Any reasoning behind why bzero is bad?
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virarptable.c | 2 +- tests/virpcimock.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
This was found by clang-tidy's "clang-analyzer-security.insecureAPI.bzero" check.
Any reasoning behind why bzero is bad?
Yeah, it is wierd to call this an insecure API. If anything memset is more dangerous because people invert the 2nd and 3rd args, resulting in not setting any bytes at all. None the less bzero is deprecated, so it makes sense to use the memset funtion in general.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virarptable.c | 2 +- tests/virpcimock.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jan 28, 2021 at 10:59:41 +0000, Daniel Berrange wrote:
On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
This was found by clang-tidy's "clang-analyzer-security.insecureAPI.bzero" check.
Any reasoning behind why bzero is bad?
Yeah, it is wierd to call this an insecure API. If anything memset is more dangerous because people invert the 2nd and 3rd args, resulting in not setting any bytes at all.
According to the manpage it can allegedly be optimized out: The explicit_bzero() function performs the same task as bzero(). It differs from bzero() in that it guarantees that compiler optimizations will not remove the erase operation if the compiler deduces that the operation is "unnecessary".
None the less bzero is deprecated, so it makes sense to use the memset funtion in general.
Yes it does, but the reason should be mentioned in the commit message.
On Thu, Jan 28, 2021 at 12:03:36PM +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 10:59:41 +0000, Daniel Berrange wrote:
On Thu, Jan 28, 2021 at 11:45:07AM +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:35 +0100, Tim Wiederhake wrote:
This was found by clang-tidy's "clang-analyzer-security.insecureAPI.bzero" check.
Any reasoning behind why bzero is bad?
Yeah, it is wierd to call this an insecure API. If anything memset is more dangerous because people invert the 2nd and 3rd args, resulting in not setting any bytes at all.
According to the manpage it can allegedly be optimized out:
The explicit_bzero() function performs the same task as bzero(). It differs from bzero() in that it guarantees that compiler optimizations will not remove the erase operation if the compiler deduces that the operation is "unnecessary".
A compiler smart enough eliminate bzero can do also likely eliminate memset.
None the less bzero is deprecated, so it makes sense to use the memset funtion in general.
Yes it does, but the reason should be mentioned in the commit message.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value". This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online". Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1; + + if (virStrToLong_i(str, NULL, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to convert '%s' to int"), str); return -1; -- 2.26.2
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value".
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
str = udevGetDeviceSysfsAttr(udev_device, attr_name);
- if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1;
In this case an error wouldn't be reported any more.
+ + if (virStrToLong_i(str, NULL, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to convert '%s' to int"), str);
while here it was
return -1; -- 2.26.2
On 1/28/21 11:44 AM, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value".
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
str = udevGetDeviceSysfsAttr(udev_device, attr_name);
- if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1;
In this case an error wouldn't be reported any more.
I think it's quite the opposite actually. Previously, if str == NULL then a zero was returned (without any error) from this function. Now you get -1. I think we want to keep return 0 in case of !str. Callers use the following pattern: var = -1; /* default */ udevGetIntSysfsAttr(device, "attribute", &var, 10); If "attribute" exists, @var is updated; if it doesn't it's left untouched with the default value (-1 in this case). Michal
On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
On 1/28/21 11:44 AM, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value".
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1;
In this case an error wouldn't be reported any more.
I think it's quite the opposite actually. Previously, if str == NULL then a zero was returned (without any error) from this function. Now you get -1.
I think we want to keep return 0 in case of !str. Callers use the following pattern:
var = -1; /* default */ udevGetIntSysfsAttr(device, "attribute", &var, 10);
If "attribute" exists, @var is updated; if it doesn't it's left untouched with the default value (-1 in this case).
There should not be any code with this pattern because that leads us to ignore genuine errors. If we want to degrade when an attribute isn't present, we must be explict about that and test existance of the sysfs file Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/28/21 1:47 PM, Daniel P. Berrangé wrote:
On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
On 1/28/21 11:44 AM, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value".
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1;
In this case an error wouldn't be reported any more.
I think it's quite the opposite actually. Previously, if str == NULL then a zero was returned (without any error) from this function. Now you get -1.
I think we want to keep return 0 in case of !str. Callers use the following pattern:
var = -1; /* default */ udevGetIntSysfsAttr(device, "attribute", &var, 10);
If "attribute" exists, @var is updated; if it doesn't it's left untouched with the default value (-1 in this case).
There should not be any code with this pattern because that leads us to ignore genuine errors.
I was a bit too harsh in my reply. Of course we check for udevGetIntSysfsAttr() retval. The pattern is like this: var = -1; if (udevGetIntSysfsAttr(device, "attribute", &var, 10) < 0) goto error; And I think this okay.
If we want to degrade when an attribute isn't present, we must be explict about that and test existance of the sysfs file
Well, the above example would then look like this: var = -1; str = udev_device_get_sysattr_value(device, "attribute"); if (str && virStrToLong_i(str, NULL, &var, 10) < 0) { /* error */ } Which is exactly what udevGetIntSysfsAttr() does, except it's open coded. Michal
On Thu, Jan 28, 2021 at 02:03:25PM +0100, Michal Privoznik wrote:
On 1/28/21 1:47 PM, Daniel P. Berrangé wrote:
On Thu, Jan 28, 2021 at 01:18:07PM +0100, Michal Privoznik wrote:
On 1/28/21 11:44 AM, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value".
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device, str = udevGetDeviceSysfsAttr(udev_device, attr_name); - if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1;
In this case an error wouldn't be reported any more.
I think it's quite the opposite actually. Previously, if str == NULL then a zero was returned (without any error) from this function. Now you get -1.
I think we want to keep return 0 in case of !str. Callers use the following pattern:
var = -1; /* default */ udevGetIntSysfsAttr(device, "attribute", &var, 10);
If "attribute" exists, @var is updated; if it doesn't it's left untouched with the default value (-1 in this case).
There should not be any code with this pattern because that leads us to ignore genuine errors.
I was a bit too harsh in my reply. Of course we check for udevGetIntSysfsAttr() retval. The pattern is like this:
var = -1; if (udevGetIntSysfsAttr(device, "attribute", &var, 10) < 0) goto error;
And I think this okay.
If we want to degrade when an attribute isn't present, we must be explict about that and test existance of the sysfs file
Well, the above example would then look like this:
var = -1; str = udev_device_get_sysattr_value(device, "attribute"); if (str && virStrToLong_i(str, NULL, &var, 10) < 0) { /* error */ }
Which is exactly what udevGetIntSysfsAttr() does, except it's open coded.
Ok, so the real bug here is idevProcessCCW not initializing the "online" variable before calling the method. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, 2021-01-28 at 11:44 +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:36 +0100, Tim Wiederhake wrote:
If "udevGetDeviceSysfsAttr()" returns NULL, "udevGetIntSysfsAttr" would return "0", indicating success, without writing to "value".
This was found by clang-tidy's "clang-analyzer-core.UndefinedBinaryOperatorResult" check in function "udevProcessCCW", flagging a read on the potentially uninitialized variable "online".
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_udev.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 55a2731681..d5a12bab0e 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -254,7 +254,10 @@ udevGetIntSysfsAttr(struct udev_device *udev_device,
str = udevGetDeviceSysfsAttr(udev_device, attr_name);
- if (str && virStrToLong_i(str, NULL, base, value) < 0) { + if (!str) + return -1;
In this case an error wouldn't be reported any more.
+ + if (virStrToLong_i(str, NULL, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to convert '%s' to int"), str);
while here it was
I believe this is not correct. Here is how the code looked before: (1) str = udevGetDeviceSysfsAttr(udev_device, attr_name); (2) if (str && virStrToLong_i(str, NULL, base, value) < 0) { (3) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to convert '%s' to int"), str); return -1; } return 0; If "udevGetDeviceSysfsAttr" returns NULL in line (1), the (non- existant) "false" branch of line (2) is taken, i.e. the virReportError in line (3) is not executed. In the new version of the code: (4) str = udevGetDeviceSysfsAttr(udev_device, attr_name); (5) if (!str) return -1; if (virStrToLong_i(str, NULL, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to convert '%s' to int"), str); return -1; } return 0; If "udevGetDeviceSysfsAttr" returns NULL in line (4), this is caught in line (5), still not reporting an error message, but returning with an appropriate return value. Reporting the error is performed higher up in the call chain, e.g.: * udevAddOneDevice ← reports the issue in "cleanup" block * udevGetDeviceDetails * udevProcessCCW * udevGetIntSysfsAttr ← now returns -1 for missing attributes Cheers, Tim
"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> --- 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index ba5681b715..9394a42726 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -99,7 +99,7 @@ int main(int argc, char **argv) { origenv++; } - if (!(newenv = malloc(sizeof(*newenv) * n))) + if ((n == 0) || !(newenv = malloc(sizeof(*newenv) * n))) abort(); origenv = environ; -- 2.26.2
On Thu, Jan 28, 2021 at 11:24:38 +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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/commandhelper.c b/tests/commandhelper.c index ba5681b715..9394a42726 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -99,7 +99,7 @@ int main(int argc, char **argv) { origenv++; }
- if (!(newenv = malloc(sizeof(*newenv) * n))) + if ((n == 0) || !(newenv = malloc(sizeof(*newenv) * n))) abort();
This doesn't seem to be an abort-worthy case. It's unlikely but the environment can contain no variables. Also it makes stuff hard to debug. If n==0 is indeed a problem an error should be reported.
origenv = environ; -- 2.26.2
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> --- 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
`val` is declared `ATTRIBUTE_NONNULL`. Found by clang-tidy's "clang-diagnostic-tautological-pointer-compare" check. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/vircommand.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c3a98bbeac..223010c6aa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val) if (!cmd || cmd->has_error) return; - if (val == NULL) { - cmd->has_error = EINVAL; - return; - } - arg = g_strdup(val); /* Arg plus trailing NULL. */ -- 2.26.2
On Thu, Jan 28, 2021 at 11:24:40 +0100, Tim Wiederhake wrote:
`val` is declared `ATTRIBUTE_NONNULL`.
Please see: https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h#L127 ATTRIBUTE_NONNULL is unfortunately lead to wrong optimizations done by gcc.
Found by clang-tidy's "clang-diagnostic-tautological-pointer-compare" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/vircommand.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c3a98bbeac..223010c6aa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val) if (!cmd || cmd->has_error) return;
- if (val == NULL) { - cmd->has_error = EINVAL; - return; - }
So this might have actual value.
On Thu, 2021-01-28 at 11:54 +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:40 +0100, Tim Wiederhake wrote:
`val` is declared `ATTRIBUTE_NONNULL`.
Please see:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h#L127
ATTRIBUTE_NONNULL is unfortunately lead to wrong optimizations done by gcc.
Found by clang-tidy's "clang-diagnostic-tautological-pointer- compare" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/vircommand.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c3a98bbeac..223010c6aa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val) if (!cmd || cmd->has_error) return;
- if (val == NULL) { - cmd->has_error = EINVAL; - return; - }
So this might have actual value.
Hm, I see. I will remove this patch from the series for now. A bit frustrating though, if you compare with, as a random example, "virCommandGetGID", which is declared in [1] as "ATTRIBUTE_NONNULL" for argument "cmd", which is subsequently dereferenced without additional NULL check in [2]. Cheers, Tim [1] https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/vircommand.h#L71 [2] https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/vircommand.c#L1116
On Thu, Jan 28, 2021 at 15:21:28 +0100, Tim Wiederhake wrote:
On Thu, 2021-01-28 at 11:54 +0100, Peter Krempa wrote:
On Thu, Jan 28, 2021 at 11:24:40 +0100, Tim Wiederhake wrote:
`val` is declared `ATTRIBUTE_NONNULL`.
Please see:
https://gitlab.com/libvirt/libvirt/-/blob/master/src/internal.h#L127
ATTRIBUTE_NONNULL is unfortunately lead to wrong optimizations done by gcc.
Found by clang-tidy's "clang-diagnostic-tautological-pointer- compare" check.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/vircommand.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c index c3a98bbeac..223010c6aa 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1525,11 +1525,6 @@ virCommandAddArg(virCommandPtr cmd, const char *val) if (!cmd || cmd->has_error) return;
- if (val == NULL) { - cmd->has_error = EINVAL; - return; - }
So this might have actual value.
Hm, I see. I will remove this patch from the series for now. A bit frustrating though, if you compare with, as a random example, "virCommandGetGID", which is declared in [1] as "ATTRIBUTE_NONNULL" for argument "cmd", which is subsequently dereferenced without additional NULL check in [2].
I consider the difference here to be on libvirt-semantic level rather than language-semantic level. Libvirt's semantics dictate that virCommandAddArg must not be used with NULL 'val' since adding a NULL argument wouldn't make sense. We similarly check this in virCommandAddArgSet that the set of arguments to add is non-empty. (You can also see a ATTRIBUTE_NONNULL there, but it works differently). The language semantics of ATTRIBUTE_NONNULL dictate that the function must not be called with NULL argument, which in case of virCommandAddArg is equivalent with the desired libvirt semantics. Unfortunately due to the weird handling in the compiler (not static analyzer) the compiler doesn't guarantee anything about the argument if ATTRIBUTE_NONNULL is used. Since the explicit check of val is required to ensure the semantics despite the uselessness of ATTRIBUTE_NONNULL when used in real compile case, we must keep the check in. So the question is now whether to keep it marked as ATTRIBUTE_NONNULL, or just do it on documentation level and appease the static analyzer by dropping ATTRIBUTE_NONNULL.
Signed-off-by: Tim Wiederhake <twiederh@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 223010c6aa..f83d49ffac 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1520,21 +1520,16 @@ virCommandAddEnvXDG(virCommandPtr cmd, const char *baseDir) void virCommandAddArg(virCommandPtr cmd, const char *val) { - char *arg; - if (!cmd || cmd->has_error) 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
On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote:
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 and the slow analysis, triggering time-outs in the CI, make this tool unfit for inclusion in libvirt's GitLab CI though.
Is it possible to make it viable for CI by disabling *all* checks by default and then selectively re-enabling just the handful that are useful and don't have false positives ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, 2021-01-28 at 10:35 +0000, Daniel P. Berrangé wrote:
On Thu, Jan 28, 2021 at 11:24:30AM +0100, Tim Wiederhake wrote:
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 and the slow analysis, triggering time-outs in the CI, make this tool unfit for inclusion in libvirt's GitLab CI though.
Is it possible to make it viable for CI by disabling *all* checks by default and then selectively re-enabling just the handful that are useful and don't have false positives ?
Regards, Daniel
That is what I tried at first. Unfortunately, it does not work for several reasons: * clang-tidy does not like memory constraint environments -- at least that appears to be the bottom of it. The result is random segfaults. * There are false positive findings in the group of "clang-analyze-*" and "clang-diagnostic-*", which cannot be disabled or rather, are implicitly re-enabled no matter your configuration. E.g: Flagging https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virfdstream.c#L124... as dead store, see above explation of "__attribute__((cleanup))" for background. * There are true positive findings in the same group of checks that I, frankly, just disagree with. E.g: Flagging https://gitlab.com/libvirt/libvirt/-/blob/master/src/util/virobject.c#L228 as tautological comparison (in the first iteration of the `while` loop), as `klass` is declared ATTRIBUTE_NONNULL. "Unrolling" the first iteration does not make the code better in my eyes. "no-lint" markers for clang-tidy do exist, but I would rather not litter the code with them to make some software happy. * clang-tidy requires all files to be present, or the run will terminate with a non-zero return code and analysis errors. This includes generated files. My Meson-fu is not particularly great, and I have not found a "run generators only" target. Building libvirt completely before running clang-tidy cuts badly in gitlab's one-hour time budget, and running the monster below, for which I would like to profoundly apologize, is also not a future-proof option: [ -d "${bld_dir}" ] || CC=clang meson "${bld_dir}" "${src_dir}" ninja -C "${bld_dir}" \ src/access/viraccessapicheck.c \ src/access/viraccessapicheck.h \ src/access/viraccessapichecklxc.c \ src/access/viraccessapichecklxc.h \ src/access/viraccessapicheckqemu.c \ src/access/viraccessapicheckqemu.h \ src/admin/admin_client.h \ src/admin/admin_protocol.c \ src/admin/admin_protocol.h \ src/admin/admin_server_dispatch_stubs.h \ src/esx/esx_vi.generated.c \ src/esx/esx_vi.generated.h \ src/esx/esx_vi_methods.generated.c \ src/esx/esx_vi_methods.generated.h \ src/esx/esx_vi_methods.generated.macro \ src/esx/esx_vi_types.generated.c \ src/esx/esx_vi_types.generated.h \ src/esx/esx_vi_types.generated.typedef \ src/esx/esx_vi_types.generated.typeenum \ src/esx/esx_vi_types.generated.typefromstring \ src/esx/esx_vi_types.generated.typetostring \ src/hyperv/hyperv_wmi_classes.generated.c \ src/hyperv/hyperv_wmi_classes.generated.h \ src/hyperv/hyperv_wmi_classes.generated.typedef \ src/libvirt_functions.stp \ src/libvirt_probes.h \ src/libvirt_probes.stp \ src/locking/lock_daemon_dispatch_stubs.h \ src/locking/lock_protocol.c \ src/locking/lock_protocol.h \ src/logging/log_daemon_dispatch_stubs.h \ src/logging/log_protocol.c \ src/logging/log_protocol.h \ src/lxc/lxc_controller_dispatch.h \ src/lxc/lxc_monitor_dispatch.h \ src/lxc/lxc_monitor_protocol.c \ src/lxc/lxc_monitor_protocol.h \ src/qemu/libvirt_qemu_probes.h \ src/qemu/libvirt_qemu_probes.stp \ src/remote/lxc_client_bodies.h \ src/remote/lxc_daemon_dispatch_stubs.h \ src/remote/lxc_protocol.c \ src/remote/lxc_protocol.h \ src/remote/qemu_client_bodies.h \ src/remote/qemu_daemon_dispatch_stubs.h \ src/remote/qemu_protocol.c \ src/remote/qemu_protocol.h \ src/remote/remote_client_bodies.h \ src/remote/remote_daemon_dispatch_stubs.h \ src/remote/remote_protocol.c \ src/remote/remote_protocol.h \ src/rpc/virkeepaliveprotocol.c \ src/rpc/virkeepaliveprotocol.h \ src/rpc/virnetprotocol.c \ src/rpc/virnetprotocol.h \ src/util/virkeycodetable_atset1.h \ src/util/virkeycodetable_atset2.h \ src/util/virkeycodetable_atset3.h \ src/util/virkeycodetable_linux.h \ src/util/virkeycodetable_osx.h \ src/util/virkeycodetable_qnum.h \ src/util/virkeycodetable_usb.h \ src/util/virkeycodetable_win32.h \ src/util/virkeycodetable_xtkbd.h \ src/util/virkeynametable_linux.h \ src/util/virkeynametable_osx.h \ src/util/virkeynametable_win32.h \ tools/wireshark/src/libvirt/keepalive.h \ tools/wireshark/src/libvirt/lxc.h \ tools/wireshark/src/libvirt/protocol.h \ tools/wireshark/src/libvirt/qemu.h \ tools/wireshark/src/libvirt/remote.h ninja -C "${bld_dir}" clang-tidy In my opinion, it might be worthwile to have a `.clang-tidy` configuration for libvirt, but running the tests is up to humans for the forseeable future, not the CI. At the very least until clang-tidy understands `__attribute__(cleanup))`, which I would love to report as a bug to clang-tidy, but the disabled "self user registration" and the amount of stale bugs that have not seen any attention in the last five years is not giving raise to hope: https://bugs.llvm.org/buglist.cgi?component=clang-tidy&order=changeddate%2Cpriority%2Cbug_severity&product=clang-tools-extra&query_format=advanced&resolution=- -- Cheers, Tim
On Thu, Jan 28, 2021 at 11:24:30 +0100, Tim Wiederhake wrote: [...]
Tim Wiederhake (11): 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() udevGetIntSysfsAttr: Return -1 for missing attributes virhostuptime: Fix rounding in uptime calculation tests: Prevent malloc with size 0 vircryptotest: Directly assign string to avoid memcpy vircommand: Remove NULL check in virCommandAddArg vircommand: Simplify virCommandAddArg
On patches with no explicit reply: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (4)
-
Daniel P. Berrangé -
Michal Privoznik -
Peter Krempa -
Tim Wiederhake