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.
This commit is contained in:
Zbigniew Jędrzejewski-Szmek 2023-01-22 22:40:25 +01:00
parent 17d16267e2
commit a142c87042
6 changed files with 260 additions and 0 deletions

View File

@ -0,0 +1,37 @@
From 2fdd12acd5c69bc952d9ca4d5ad796e6e830d21b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= <crodriguez@owncloud.com>
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

View File

@ -0,0 +1,34 @@
From b1b7667a44c4e8635b6d8dc070fb2446187fcdc5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= <crodriguez@owncloud.com>
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

View File

@ -0,0 +1,31 @@
From ba5f7915d25a400f0651bc9e8546a3ec6a738eaa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cristian=20Rodr=C3=ADguez?= <crodriguez@owncloud.com>
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

View File

@ -0,0 +1,104 @@
From 34b9eddfc12936917fab000b780a451d6277c2b4 Mon Sep 17 00:00:00 2001
From: Siddhesh Poyarekar <siddhesh@gotplt.org>
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 <alloca.h>
+#include <malloc.h>
#include <stddef.h>
#include <stdlib.h>
#include <string.h>
@@ -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

View File

@ -0,0 +1,48 @@
From e998c9d7c1a52ab02ff6e9c363c1cfe0b76cd6f4 Mon Sep 17 00:00:00 2001
From: Siddhesh Poyarekar <siddhesh@gotplt.org>
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

View File

@ -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