The next commit will use this macro outside the apparmor context. Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com&gt; --- 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 a %A in %Y, Peter Krempa wrote: NACK to putting the value in the constant name. Jano

On Wed, Jul 21, 2021 at 14:46:41 +0200, Tim Wiederhake wrote: In this case it's okay as is ... ... 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.

On Wed, Jul 21, 2021 at 14:46:42 +0200, Tim Wiederhake wrote: 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.

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(a)redhat.com&gt; --- .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 03:08:02PM +0200, Peter Krempa wrote: 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, Jul 21, 2021 at 03:08:02PM +0200, Peter Krempa wrote: 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. 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 :|