[libvirt PATCH 0/4] CI: Fail tests on UBSAN findings
The undefined behaviour sanitizer (UBSAN) defaults to merely printing an error message if it detects undefined behaviour. These error messages often end up in captured output and do not fail the tests, effectively hiding the warning. This series fixes all previously hidden issues and makes UBSAN fail the tests in the CI, if it detects undefined behaviour. See https://listman.redhat.com/archives/libvir-list/2021-June/msg00341.html for background on the change to virFileReadLimFD usage. Tim Wiederhake (4): virfile: Move max file size macro virfile: Use VIR_MAX_FILE_LEN instead of INT_MAX to limit file size virFileReadLimFD: Limit maximum file size to INT_MAX - 1 ci: Halt on sanitizer errors .gitlab-ci.yml | 2 ++ src/security/security_apparmor.c | 4 ++-- src/security/security_apparmor.h | 1 - src/security/virt-aa-helper.c | 10 +++++----- src/util/virfile.c | 2 +- src/util/virfile.h | 3 +++ tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 5 +++-- 8 files changed, 17 insertions(+), 12 deletions(-) -- 2.31.1
The next commit will use this macro outside the apparmor context. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/security/security_apparmor.c | 4 ++-- src/security/security_apparmor.h | 1 - src/security/virt-aa-helper.c | 10 +++++----- src/util/virfile.h | 3 +++ 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 84363015dc..c2cae43137 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -83,7 +83,7 @@ profile_status(const char *str, const int check_enforcing) etmp = g_strdup_printf("%s (enforce)", str); } - if (virFileReadAll(APPARMOR_PROFILES_PATH, MAX_FILE_LEN, &content) < 0) { + if (virFileReadAll(APPARMOR_PROFILES_PATH, VIR_MAX_FILE_LEN, &content) < 0) { virReportSystemError(errno, _("Failed to read AppArmor profiles list " "\'%s\'"), APPARMOR_PROFILES_PATH); @@ -131,7 +131,7 @@ profile_status_file(const char *str) if (!virFileExists(profile)) goto failed; - if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { + if ((len = virFileReadAll(profile, VIR_MAX_FILE_LEN, &content)) < 0) { virReportSystemError(errno, _("Failed to read \'%s\'"), profile); goto failed; diff --git a/src/security/security_apparmor.h b/src/security/security_apparmor.h index 7b54eefd8d..ceffa30f14 100644 --- a/src/security/security_apparmor.h +++ b/src/security/security_apparmor.h @@ -24,4 +24,3 @@ extern virSecurityDriver virAppArmorSecurityDriver; #define AA_PREFIX "libvirt-" #define PROFILE_NAME_SIZE 8 + VIR_UUID_STRING_BUFLEN /* AA_PREFIX + uuid */ -#define MAX_FILE_LEN (1024*1024*10) /* 10MB limit for sanity check */ diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e21557c810..0db2248a59 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -211,7 +211,7 @@ update_include_file(const char *include_file, const char *included_files, "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n"; if (virFileExists(include_file)) { - flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing); + flen = virFileReadAll(include_file, VIR_MAX_FILE_LEN, &existing); if (flen < 0) return rc; } @@ -222,7 +222,7 @@ update_include_file(const char *include_file, const char *included_files, pcontent = g_strdup_printf("%s%s", warning, included_files); plen = strlen(pcontent); - if (plen > MAX_FILE_LEN) { + if (plen > VIR_MAX_FILE_LEN) { vah_error(NULL, 0, _("invalid length for new profile")); goto cleanup; } @@ -299,7 +299,7 @@ create_profile(const char *profile, const char *profile_name, return -1; } - if ((tlen = virFileReadAll(template, MAX_FILE_LEN, &tcontent)) < 0) { + if ((tlen = virFileReadAll(template, VIR_MAX_FILE_LEN, &tcontent)) < 0) { vah_error(NULL, 0, _("failed to read AppArmor template")); return -1; } @@ -326,7 +326,7 @@ create_profile(const char *profile, const char *profile_name, if (virtType != VIR_DOMAIN_VIRT_LXC) plen += strlen(replace_files) - strlen(template_end); - if (plen > MAX_FILE_LEN || plen < tlen) { + if (plen > VIR_MAX_FILE_LEN || plen < tlen) { vah_error(NULL, 0, _("invalid length for new profile")); return -1; } @@ -1429,7 +1429,7 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) if (ctl->cmd == 'c' || ctl->cmd == 'r') { char *xmlStr = NULL; - if (virFileReadLimFD(STDIN_FILENO, MAX_FILE_LEN, &xmlStr) < 0) + if (virFileReadLimFD(STDIN_FILENO, VIR_MAX_FILE_LEN, &xmlStr) < 0) vah_error(ctl, 1, _("could not read xml file")); if (get_definition(ctl, xmlStr) != 0 || ctl->def == NULL) { diff --git a/src/util/virfile.h b/src/util/virfile.h index 72368495bf..b6bcd1257d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -42,6 +42,9 @@ typedef enum { # define VIR_FILE_MODULE_EXT ".so" #endif +/* 10MB limit for sanity check */ +#define VIR_MAX_FILE_LEN (10 * 1024 * 1024) + ssize_t saferead(int fd, void *buf, size_t count) G_GNUC_WARN_UNUSED_RESULT; ssize_t safewrite(int fd, const void *buf, size_t count) G_GNUC_WARN_UNUSED_RESULT; -- 2.31.1
On Wed, Jul 21, 2021 at 14:46:40 +0200, Tim Wiederhake wrote:
The next commit will use this macro outside the apparmor context.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/security/security_apparmor.c | 4 ++-- src/security/security_apparmor.h | 1 - src/security/virt-aa-helper.c | 10 +++++----- src/util/virfile.h | 3 +++ 4 files changed, 10 insertions(+), 8 deletions(-)
[...]
diff --git a/src/util/virfile.h b/src/util/virfile.h index 72368495bf..b6bcd1257d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -42,6 +42,9 @@ typedef enum { # define VIR_FILE_MODULE_EXT ".so" #endif
+/* 10MB limit for sanity check */ +#define VIR_MAX_FILE_LEN (10 * 1024 * 1024)
Here you should add a note that this limit is arbitrary and it isn't an inherent limit of the file access functions where it's going to be used. Or alternatively pick a different name for it (VIR_MAX_FILE_LEN_10M?), since the name implies that it's an inherent limit. No need to repost, just propose what solution you'd like.
On a %A in %Y, Peter Krempa wrote:
On Wed, Jul 21, 2021 at 14:46:40 +0200, Tim Wiederhake wrote:
The next commit will use this macro outside the apparmor context.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/security/security_apparmor.c | 4 ++-- src/security/security_apparmor.h | 1 - src/security/virt-aa-helper.c | 10 +++++----- src/util/virfile.h | 3 +++ 4 files changed, 10 insertions(+), 8 deletions(-)
[...]
diff --git a/src/util/virfile.h b/src/util/virfile.h index 72368495bf..b6bcd1257d 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -42,6 +42,9 @@ typedef enum { # define VIR_FILE_MODULE_EXT ".so" #endif
+/* 10MB limit for sanity check */ +#define VIR_MAX_FILE_LEN (10 * 1024 * 1024)
Here you should add a note that this limit is arbitrary and it isn't an inherent limit of the file access functions where it's going to be used.
Or alternatively pick a different name for it (VIR_MAX_FILE_LEN_10M?), since the name implies that it's an inherent limit.
NACK to putting the value in the constant name. Jano
No need to repost, just propose what solution you'd like.
The use of INT_MAX as maximum file length is problematic for reasons discussed in the next commit. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 91336a0c55..114e17fbbd 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -176,7 +176,7 @@ mymain(void) basefile = g_strdup_printf("%s/networkxml2firewalldata/base.args", abs_srcdir); - if (virFileReadAll(basefile, INT_MAX, &baseargs) < 0) + if (virFileReadAll(basefile, VIR_MAX_FILE_LEN, &baseargs) < 0) return EXIT_FAILURE; DO_TEST("nat-default"); diff --git a/tests/testutils.c b/tests/testutils.c index 7d87e30a5c..c71e099bf1 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -313,7 +313,7 @@ virTestLoadFileJSON(const char *p, ...) if (!(path = virTestLoadFileGetPath(p, ap))) goto cleanup; - if (virFileReadAll(path, INT_MAX, &jsonstr) < 0) + if (virFileReadAll(path, VIR_MAX_FILE_LEN, &jsonstr) < 0) goto cleanup; if (!(ret = virJSONValueFromString(jsonstr))) @@ -562,7 +562,8 @@ virTestCompareToFileFull(const char *actual, if (virTestLoadFile(filename, &filecontent) < 0 && !virTestGetRegenerate()) return -1; } else { - if (virFileReadAll(filename, INT_MAX, &filecontent) < 0 && !virTestGetRegenerate()) + if (virFileReadAll(filename, VIR_MAX_FILE_LEN, &filecontent) < 0 && + !virTestGetRegenerate()) return -1; } -- 2.31.1
On Wed, Jul 21, 2021 at 14:46:41 +0200, Tim Wiederhake wrote:
The use of INT_MAX as maximum file length is problematic for reasons discussed in the next commit.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- tests/networkxml2firewalltest.c | 2 +- tests/testutils.c | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 91336a0c55..114e17fbbd 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -176,7 +176,7 @@ mymain(void)
basefile = g_strdup_printf("%s/networkxml2firewalldata/base.args", abs_srcdir);
- if (virFileReadAll(basefile, INT_MAX, &baseargs) < 0) + if (virFileReadAll(basefile, VIR_MAX_FILE_LEN, &baseargs) < 0) return EXIT_FAILURE;
DO_TEST("nat-default");
In this case it's okay as is ...
diff --git a/tests/testutils.c b/tests/testutils.c index 7d87e30a5c..c71e099bf1 100644 --- a/tests/testutils.c +++ b/tests/testutils.c @@ -313,7 +313,7 @@ virTestLoadFileJSON(const char *p, ...) if (!(path = virTestLoadFileGetPath(p, ap))) goto cleanup;
- if (virFileReadAll(path, INT_MAX, &jsonstr) < 0) + if (virFileReadAll(path, VIR_MAX_FILE_LEN, &jsonstr) < 0) goto cleanup;
if (!(ret = virJSONValueFromString(jsonstr))) @@ -562,7 +562,8 @@ virTestCompareToFileFull(const char *actual, if (virTestLoadFile(filename, &filecontent) < 0 && !virTestGetRegenerate()) return -1; } else { - if (virFileReadAll(filename, INT_MAX, &filecontent) < 0 && !virTestGetRegenerate()) + if (virFileReadAll(filename, VIR_MAX_FILE_LEN, &filecontent) < 0 && + !virTestGetRegenerate()) return -1; }
... but these two are common helper functions so having an limit of 10M is in the realms of possibility to be hit. The comments for both of those functions should mention that there's an arbitrary limit in place.
virFileReadLimFD always returns null-terminated data. To that end, it has to add one to the maximum file size. If the maxium file size is INT_MAX, this triggers a signed integer overflow. There is no instance left where a caller would call virFileReadLimFD with a maxium file size of INT_MAX. Make virFileReadLimFD error out if the maximum file size is INT_MAX to prevent the reintroduction of this issue. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 723e1ca6e5..b5600658d5 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1418,7 +1418,7 @@ virFileReadLimFD(int fd, int maxlen, char **buf) size_t len; char *s; - if (maxlen <= 0) { + if ((maxlen <= 0) || (maxlen >= INT_MAX)) { errno = EINVAL; return -1; } -- 2.31.1
On Wed, Jul 21, 2021 at 14:46:42 +0200, Tim Wiederhake wrote:
virFileReadLimFD always returns null-terminated data. To that end, it has to add one to the maximum file size. If the maxium file size is INT_MAX, this triggers a signed integer overflow.
There is no instance left where a caller would call virFileReadLimFD with a maxium file size of INT_MAX. Make virFileReadLimFD error out if the maximum file size is INT_MAX to prevent the reintroduction of this issue.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 723e1ca6e5..b5600658d5 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1418,7 +1418,7 @@ virFileReadLimFD(int fd, int maxlen, char **buf) size_t len; char *s;
- if (maxlen <= 0) { + if ((maxlen <= 0) || (maxlen >= INT_MAX)) { errno = EINVAL; return -1;
While '< 0' is common sense here, limiting to INT_MAX -1 should be mentioned in the docs. Or better, why aren't we converting this to 'size_t' instead? saferead_lim is already operating on 'size_t' and I think we could simply get rid of the overflow checks altogether when working with size_t.
On Wed, Jul 21, 2021 at 03:02:59PM +0200, Peter Krempa wrote:
On Wed, Jul 21, 2021 at 14:46:42 +0200, Tim Wiederhake wrote:
virFileReadLimFD always returns null-terminated data. To that end, it has to add one to the maximum file size. If the maxium file size is INT_MAX, this triggers a signed integer overflow.
There is no instance left where a caller would call virFileReadLimFD with a maxium file size of INT_MAX. Make virFileReadLimFD error out if the maximum file size is INT_MAX to prevent the reintroduction of this issue.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c index 723e1ca6e5..b5600658d5 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1418,7 +1418,7 @@ virFileReadLimFD(int fd, int maxlen, char **buf) size_t len; char *s;
- if (maxlen <= 0) { + if ((maxlen <= 0) || (maxlen >= INT_MAX)) { errno = EINVAL; return -1;
While '< 0' is common sense here, limiting to INT_MAX -1 should be mentioned in the docs.
Or better, why aren't we converting this to 'size_t' instead?
saferead_lim is already operating on 'size_t' and I think we could simply get rid of the overflow checks altogether when working with size_t.
Honestly we could probably eliminate essentially all use of this method. The only scenario in which we need to limit the amount of data we read from a file is for untrustworthy input files controlled by a user with lower privilege level than libvirt. There might be such a case somewhere, but in usage I've looked at so far it is not required, as the files are generally all owned y the same user, or by root, or are exposed by the kernel as magic sysfs files. The "maxlen" parameter is something I'm to blame for introducing years ago in commit 94e49e3f0e82b8b059d71b131f5e6be2a1d6de2d Author: Daniel P. Berrangé <berrange@redhat.com> Date: Mon Jan 7 15:21:33 2008 +0000 Fix config file reading to not truncate large files Before this commit there was a buflen parameter that was genuinely needed because we were reading into a preallocated buffer. My change switched to dynamically allocated buffers, but kept a limit for reasons I can't really justify today. 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 :|
The undefined behaviour sanitizer (UBSAN) defaults to merely printing an error message if it detects undefined behaviour. These error messages often end up in captured output and do not fail the tests, effectively hiding the warning. Make the test cases fail to make the issues visible. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3cb6ff5e6b..4757139fa9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -89,6 +89,8 @@ stages: - meson build --werror -Ddocs=disabled -Db_lundef=false -Db_sanitize="$SANITIZER" - ninja -C build; - ninja -C build test; + variables: + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1 # Jobs that we delegate to Cirrus CI because they require an operating # system other than Linux. These jobs will only run if the required -- 2.31.1
On Wed, Jul 21, 2021 at 14:46:43 +0200, Tim Wiederhake wrote:
The undefined behaviour sanitizer (UBSAN) defaults to merely printing an error message if it detects undefined behaviour. These error messages often end up in captured output and do not fail the tests, effectively hiding the warning. Make the test cases fail to make the issues visible.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3cb6ff5e6b..4757139fa9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -89,6 +89,8 @@ stages: - meson build --werror -Ddocs=disabled -Db_lundef=false -Db_sanitize="$SANITIZER" - ninja -C build; - ninja -C build test; + variables: + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1
Is this being propagated as an env variable? In many cases in the gitlab-ci there are entries doing 'export ENV=VAL' for some reason. If this really results in env variables then: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Wed, Jul 21, 2021 at 03:08:02PM +0200, Peter Krempa wrote:
On Wed, Jul 21, 2021 at 14:46:43 +0200, Tim Wiederhake wrote:
+++ b/.gitlab-ci.yml @@ -89,6 +89,8 @@ stages: - meson build --werror -Ddocs=disabled -Db_lundef=false -Db_sanitize="$SANITIZER" - ninja -C build; - ninja -C build test; + variables: + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1
Is this being propagated as an env variable? In many cases in the gitlab-ci there are entries doing 'export ENV=VAL' for some reason.
Setting environment variables as part of the script rather than using the native GitLab CI keyword is necessary to be able to use the same environment for multiple jobs: using something like .template: variables: FOO: foo job: extends: .template variables: BAR: bar would result in only $BAR being set for job, which is not what we want. We always use 'variables' where possible. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2021-07-21 at 06:56 -0700, Andrea Bolognani wrote:
On Wed, Jul 21, 2021 at 03:08:02PM +0200, Peter Krempa wrote:
On Wed, Jul 21, 2021 at 14:46:43 +0200, Tim Wiederhake wrote:
+++ b/.gitlab-ci.yml @@ -89,6 +89,8 @@ stages: - meson build --werror -Ddocs=disabled -Db_lundef=false - Db_sanitize="$SANITIZER" - ninja -C build; - ninja -C build test; + variables: + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1
Is this being propagated as an env variable? In many cases in the gitlab-ci there are entries doing 'export ENV=VAL' for some reason.
Setting environment variables as part of the script rather than using the native GitLab CI keyword is necessary to be able to use the same environment for multiple jobs: using something like
.template: variables: FOO: foo
job: extends: .template variables: BAR: bar
would result in only $BAR being set for job, which is not what we want. We always use 'variables' where possible.
I am not sure that I understand you correctly: .gitlab-ci.yml: .template: variables: MARKER_FOO: foo script: "env | grep MARKER" job: extends: .template variables: MARKER_BAR: bar Both "MARKER_FOO" and "MARKER_BAR" are set: https://gitlab.com/twiederh/libvirt/-/jobs/1443690227 Here is a pipeline for a branch with only patch #4, which fails as expected: https://gitlab.com/twiederh/libvirt/-/pipelines/340558056 Is it possible that that Gitlab used to exhibit the behavior that you describe, but it was fixed in the meantime? Regards, Tim
On Wed, Jul 21, 2021 at 03:08:02PM +0200, Peter Krempa wrote:
On Wed, Jul 21, 2021 at 14:46:43 +0200, Tim Wiederhake wrote:
The undefined behaviour sanitizer (UBSAN) defaults to merely printing an error message if it detects undefined behaviour. These error messages often end up in captured output and do not fail the tests, effectively hiding the warning. Make the test cases fail to make the issues visible.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- .gitlab-ci.yml | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 3cb6ff5e6b..4757139fa9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -89,6 +89,8 @@ stages: - meson build --werror -Ddocs=disabled -Db_lundef=false -Db_sanitize="$SANITIZER" - ninja -C build; - ninja -C build test; + variables: + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1
Is this being propagated as an env variable? In many cases in the gitlab-ci there are entries doing 'export ENV=VAL' for some reason.
Some of the exported variables rely on state provided by the shell ($pwd) or by the running container image ($CCACHE_WRAPPERSDIR), so they canot be declared in the "variables:" block.
If this really results in env variables then:
Yes, it does. 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 :|
participants (5)
-
Andrea Bolognani -
Daniel P. Berrangé -
Jano Tomko -
Peter Krempa -
Tim Wiederhake