From a142c87042e93093cc5860620c4ad99bbbae92e7 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Jan 22 2023 21:41:22 +0000 Subject: Backport patches to fix issues gcc-13 and -D_FORTIFY_SOURCE=3 gcc has a new warning which caught a bug of int/enum mismatches. And we would crash on some architectures when built with -D_FORTIFY_SOURCE=3 because of our malloc_usable_size() use. This should resolve the build failure in F38 mass build. --- diff --git a/0001-shared-install-Use-InstallChangeType-consistently.patch b/0001-shared-install-Use-InstallChangeType-consistently.patch new file mode 100644 index 0000000..41ce82b --- /dev/null +++ b/0001-shared-install-Use-InstallChangeType-consistently.patch @@ -0,0 +1,37 @@ +From 2fdd12acd5c69bc952d9ca4d5ad796e6e830d21b Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= +Date: Fri, 11 Nov 2022 15:34:32 +0000 +Subject: [PATCH 1/5] shared|install: Use InstallChangeType consistently + +gcc 13 -Wenum-int-mismatch, enabled by default, reminds us enum ! = int + +(cherry picked from commit 9264db1a0ac6034ab5b40ef3f5914d8dc7d77aba) +--- + src/shared/install.h | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/shared/install.h b/src/shared/install.h +index 9bb412ba06..0abc73897e 100644 +--- a/src/shared/install.h ++++ b/src/shared/install.h +@@ -197,7 +197,7 @@ int unit_file_exists(LookupScope scope, const LookupPaths *paths, const char *na + int unit_file_get_list(LookupScope scope, const char *root_dir, Hashmap *h, char **states, char **patterns); + Hashmap* unit_file_list_free(Hashmap *h); + +-InstallChangeType install_changes_add(InstallChange **changes, size_t *n_changes, int type, const char *path, const char *source); ++InstallChangeType install_changes_add(InstallChange **changes, size_t *n_changes, InstallChangeType type, const char *path, const char *source); + void install_changes_free(InstallChange *changes, size_t n_changes); + void install_changes_dump(int r, const char *verb, const InstallChange *changes, size_t n_changes, bool quiet); + +@@ -224,7 +224,7 @@ UnitFileState unit_file_state_from_string(const char *s) _pure_; + /* from_string conversion is unreliable because of the overlap between -EPERM and -1 for error. */ + + const char *install_change_type_to_string(InstallChangeType t) _const_; +-int install_change_type_from_string(const char *s) _pure_; ++InstallChangeType install_change_type_from_string(const char *s) _pure_; + + const char *unit_file_preset_mode_to_string(UnitFilePresetMode m) _const_; + UnitFilePresetMode unit_file_preset_mode_from_string(const char *s) _pure_; +-- +2.39.1 + diff --git a/0002-journal-remote-code-is-of-type-enum-MHD_RequestTermi.patch b/0002-journal-remote-code-is-of-type-enum-MHD_RequestTermi.patch new file mode 100644 index 0000000..3a86af1 --- /dev/null +++ b/0002-journal-remote-code-is-of-type-enum-MHD_RequestTermi.patch @@ -0,0 +1,34 @@ +From b1b7667a44c4e8635b6d8dc070fb2446187fcdc5 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= +Date: Fri, 11 Nov 2022 15:28:51 +0000 +Subject: [PATCH 2/5] journal-remote: code is of type enum + MHD_RequestTerminationCode + +Fixes gcc 13 -Wenum-int-mismatch which are enabled by default. + +(cherry picked from commit aa70dd624bff6280ab6f2871f62d313bdb1e1bcc) +--- + src/journal-remote/microhttpd-util.h | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/journal-remote/microhttpd-util.h b/src/journal-remote/microhttpd-util.h +index 7e7d1b56b1..df18335469 100644 +--- a/src/journal-remote/microhttpd-util.h ++++ b/src/journal-remote/microhttpd-util.h +@@ -64,11 +64,11 @@ void microhttpd_logger(void *arg, const char *fmt, va_list ap) _printf_(2, 0); + + int mhd_respondf(struct MHD_Connection *connection, + int error, +- unsigned code, ++ enum MHD_RequestTerminationCode code, + const char *format, ...) _printf_(4,5); + + int mhd_respond(struct MHD_Connection *connection, +- unsigned code, ++ enum MHD_RequestTerminationCode code, + const char *message); + + int mhd_respond_oom(struct MHD_Connection *connection); +-- +2.39.1 + diff --git a/0003-resolve-dns_server_feature_level_-_string-type-is-Dn.patch b/0003-resolve-dns_server_feature_level_-_string-type-is-Dn.patch new file mode 100644 index 0000000..d328854 --- /dev/null +++ b/0003-resolve-dns_server_feature_level_-_string-type-is-Dn.patch @@ -0,0 +1,31 @@ +From ba5f7915d25a400f0651bc9e8546a3ec6a738eaa Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= +Date: Fri, 11 Nov 2022 15:31:18 +0000 +Subject: [PATCH 3/5] resolve: dns_server_feature_level_*_string type is + DnsServerFeatureLevel + +gcc 13 -Wenum-int-mismatch reminds us that enum != int + +(cherry picked from commit e14afe31c3e8380496dc85b57103b2f648bc7d43) +--- + src/resolve/resolved-dns-server.h | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h +index be9efb0a79..f939b534c3 100644 +--- a/src/resolve/resolved-dns-server.h ++++ b/src/resolve/resolved-dns-server.h +@@ -44,8 +44,8 @@ typedef enum DnsServerFeatureLevel { + #define DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(x) ((x) >= DNS_SERVER_FEATURE_LEVEL_DO) + #define DNS_SERVER_FEATURE_LEVEL_IS_UDP(x) IN_SET(x, DNS_SERVER_FEATURE_LEVEL_UDP, DNS_SERVER_FEATURE_LEVEL_EDNS0, DNS_SERVER_FEATURE_LEVEL_DO) + +-const char* dns_server_feature_level_to_string(int i) _const_; +-int dns_server_feature_level_from_string(const char *s) _pure_; ++const char* dns_server_feature_level_to_string(DnsServerFeatureLevel i) _const_; ++DnsServerFeatureLevel dns_server_feature_level_from_string(const char *s) _pure_; + + struct DnsServer { + Manager *manager; +-- +2.39.1 + diff --git a/0004-Use-dummy-allocator-to-make-accesses-defined-as-per-.patch b/0004-Use-dummy-allocator-to-make-accesses-defined-as-per-.patch new file mode 100644 index 0000000..516f45c --- /dev/null +++ b/0004-Use-dummy-allocator-to-make-accesses-defined-as-per-.patch @@ -0,0 +1,104 @@ +From 34b9eddfc12936917fab000b780a451d6277c2b4 Mon Sep 17 00:00:00 2001 +From: Siddhesh Poyarekar +Date: Tue, 13 Dec 2022 16:54:36 -0500 +Subject: [PATCH 4/5] Use dummy allocator to make accesses defined as per + standard + +systemd uses malloc_usable_size() everywhere to use memory blocks +obtained through malloc, but that is abuse since the +malloc_usable_size() interface isn't meant for this kind of use, it is +for diagnostics only. This is also why systemd behaviour is flaky when +built with _FORTIFY_SOURCE. + +One way to make this more standard (and hence safer) is to, at every +malloc_usable_size() call, also 'reallocate' the block so that the +compiler can see the larger size. This is done through a dummy +reallocator whose only purpose is to tell the compiler about the larger +usable size, it doesn't do any actual reallocation. + +Florian Weimer pointed out that this doesn't solve the problem of an +allocator potentially growing usable size at will, which will break the +implicit assumption in systemd use that the value returned remains +constant as long as the object is valid. The safest way to fix that is +for systemd to step away from using malloc_usable_size() like this. + +Resolves #22801. + +(cherry picked from commit 7929e180aa47a2692ad4f053afac2857d7198758) +--- + src/basic/alloc-util.c | 4 ++++ + src/basic/alloc-util.h | 38 ++++++++++++++++++++++++++++---------- + 2 files changed, 32 insertions(+), 10 deletions(-) + +diff --git a/src/basic/alloc-util.c b/src/basic/alloc-util.c +index b030f454b2..6063943c88 100644 +--- a/src/basic/alloc-util.c ++++ b/src/basic/alloc-util.c +@@ -102,3 +102,7 @@ void* greedy_realloc0( + + return q; + } ++ ++void *expand_to_usable(void *ptr, size_t newsize _unused_) { ++ return ptr; ++} +diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h +index b38db7d473..eb53aae6f3 100644 +--- a/src/basic/alloc-util.h ++++ b/src/basic/alloc-util.h +@@ -2,6 +2,7 @@ + #pragma once + + #include ++#include + #include + #include + #include +@@ -184,17 +185,34 @@ void* greedy_realloc0(void **p, size_t need, size_t size); + # define msan_unpoison(r, s) + #endif + +-/* This returns the number of usable bytes in a malloc()ed region as per malloc_usable_size(), in a way that +- * is compatible with _FORTIFY_SOURCES. If _FORTIFY_SOURCES is used many memory operations will take the +- * object size as returned by __builtin_object_size() into account. Hence, let's return the smaller size of +- * malloc_usable_size() and __builtin_object_size() here, so that we definitely operate in safe territory by +- * both the compiler's and libc's standards. Note that __builtin_object_size() evaluates to SIZE_MAX if the +- * size cannot be determined, hence the MIN() expression should be safe with dynamically sized memory, +- * too. Moreover, when NULL is passed malloc_usable_size() is documented to return zero, and +- * __builtin_object_size() returns SIZE_MAX too, hence we also return a sensible value of 0 in this corner +- * case. */ ++/* Dummy allocator to tell the compiler that the new size of p is newsize. The implementation returns the ++ * pointer as is; the only reason for its existence is as a conduit for the _alloc_ attribute. This cannot be ++ * a static inline because gcc then loses the attributes on the function. ++ * See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 */ ++void *expand_to_usable(void *p, size_t newsize) _alloc_(2) _returns_nonnull_; ++ ++static inline size_t malloc_sizeof_safe(void **xp) { ++ if (_unlikely_(!xp || !*xp)) ++ return 0; ++ ++ size_t sz = malloc_usable_size(*xp); ++ *xp = expand_to_usable(*xp, sz); ++ /* GCC doesn't see the _returns_nonnull_ when built with ubsan, so yet another hint to make it doubly ++ * clear that expand_to_usable won't return NULL. ++ * See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79265 */ ++ if (!*xp) ++ assert_not_reached(); ++ return sz; ++} ++ ++/* This returns the number of usable bytes in a malloc()ed region as per malloc_usable_size(), which may ++ * return a value larger than the size that was actually allocated. Access to that additional memory is ++ * discouraged because it violates the C standard; a compiler cannot see that this as valid. To help the ++ * compiler out, the MALLOC_SIZEOF_SAFE macro 'allocates' the usable size using a dummy allocator function ++ * expand_to_usable. There is a possibility of malloc_usable_size() returning different values during the ++ * lifetime of an object, which may cause problems, but the glibc allocator does not do that at the moment. */ + #define MALLOC_SIZEOF_SAFE(x) \ +- MIN(malloc_usable_size(x), __builtin_object_size(x, 0)) ++ malloc_sizeof_safe((void**) &__builtin_choose_expr(__builtin_constant_p(x), (void*) { NULL }, (x))) + + /* Inspired by ELEMENTSOF() but operates on malloc()'ed memory areas: typesafely returns the number of items + * that fit into the specified memory block */ +-- +2.39.1 + diff --git a/0005-alloc-util-Disallow-inlining-of-expand_to_usable.patch b/0005-alloc-util-Disallow-inlining-of-expand_to_usable.patch new file mode 100644 index 0000000..0ab4473 --- /dev/null +++ b/0005-alloc-util-Disallow-inlining-of-expand_to_usable.patch @@ -0,0 +1,48 @@ +From e998c9d7c1a52ab02ff6e9c363c1cfe0b76cd6f4 Mon Sep 17 00:00:00 2001 +From: Siddhesh Poyarekar +Date: Sat, 7 Jan 2023 19:30:32 -0500 +Subject: [PATCH 5/5] alloc-util: Disallow inlining of expand_to_usable + +Explicitly set __attribute__ ((noinline)) so that the compiler does not +attempt to inline expand_to_usable, even with LTO. + +(cherry picked from commit 4f79f545b3c46c358666c9f5f2b384fe50aac4b4) +--- + src/basic/alloc-util.h | 7 ++++--- + src/fundamental/macro-fundamental.h | 1 + + 2 files changed, 5 insertions(+), 3 deletions(-) + +diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h +index eb53aae6f3..bf783b15a2 100644 +--- a/src/basic/alloc-util.h ++++ b/src/basic/alloc-util.h +@@ -186,10 +186,11 @@ void* greedy_realloc0(void **p, size_t need, size_t size); + #endif + + /* Dummy allocator to tell the compiler that the new size of p is newsize. The implementation returns the +- * pointer as is; the only reason for its existence is as a conduit for the _alloc_ attribute. This cannot be +- * a static inline because gcc then loses the attributes on the function. ++ * pointer as is; the only reason for its existence is as a conduit for the _alloc_ attribute. This must not ++ * be inlined (hence a non-static function with _noinline_ because LTO otherwise tries to inline it) because ++ * gcc then loses the attributes on the function. + * See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 */ +-void *expand_to_usable(void *p, size_t newsize) _alloc_(2) _returns_nonnull_; ++void *expand_to_usable(void *p, size_t newsize) _alloc_(2) _returns_nonnull_ _noinline_; + + static inline size_t malloc_sizeof_safe(void **xp) { + if (_unlikely_(!xp || !*xp)) +diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h +index c11a5b15f4..e73174a593 100644 +--- a/src/fundamental/macro-fundamental.h ++++ b/src/fundamental/macro-fundamental.h +@@ -20,6 +20,7 @@ + #define _hidden_ __attribute__((__visibility__("hidden"))) + #define _likely_(x) (__builtin_expect(!!(x), 1)) + #define _malloc_ __attribute__((__malloc__)) ++#define _noinline_ __attribute__((noinline)) + #define _noreturn_ _Noreturn + #define _packed_ __attribute__((__packed__)) + #define _printf_(a, b) __attribute__((__format__(printf, a, b))) +-- +2.39.1 + diff --git a/systemd.spec b/systemd.spec index 555abb9..a48ae37 100644 --- a/systemd.spec +++ b/systemd.spec @@ -92,6 +92,12 @@ Patch0001: 0001-pam-align-second-and-third-columns.patch Patch0002: 0002-pam-add-a-call-to-pam_namespace.patch Patch0003: 0003-pam-actually-align-the-columns.patch +Patch0011: 0001-shared-install-Use-InstallChangeType-consistently.patch +Patch0012: 0002-journal-remote-code-is-of-type-enum-MHD_RequestTermi.patch +Patch0013: 0003-resolve-dns_server_feature_level_-_string-type-is-Dn.patch +Patch0014: 0004-Use-dummy-allocator-to-make-accesses-defined-as-per-.patch +Patch0015: 0005-alloc-util-Disallow-inlining-of-expand_to_usable.patch + # Those are downstream-only patches, but we don't want them in packit builds: # https://bugzilla.redhat.com/show_bug.cgi?id=1738828 Patch0490: use-bfq-scheduler.patch