From ed2506d1d077431713308311bafedfc4170cd19a Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Sun, 20 Apr 2025 12:24:09 +0200 Subject: [PATCH 1/7] boot: Make free() a non inline function The static inline declaration in iovec-fundamental.h causes static analyze tooling warnings (clang-tidy). Let's get around it by making free() non-inline. LTO will make sure it's still inlined properly. --- src/boot/util.c | 13 +++++++++++++ src/boot/util.h | 15 +++------------ src/fundamental/iovec-util-fundamental.h | 2 +- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/src/boot/util.c b/src/boot/util.c index 5dd111e7c5..a218ee2cc1 100644 --- a/src/boot/util.c +++ b/src/boot/util.c @@ -13,6 +13,19 @@ /* Never try to read more than 16G into memory (and on 32bit 1G) */ #define FILE_READ_MAX MIN(SIZE_MAX/4, UINT64_C(16)*1024U*1024U*1024U) +void free(void *p) { + if (!p) + return; + + /* Debugging an invalid free requires trace logging to find the call site or a debugger attached. For + * release builds it is not worth the bother to even warn when we cannot even print a call stack. */ +#ifdef EFI_DEBUG + assert_se(BS->FreePool(p) == EFI_SUCCESS); +#else + (void) BS->FreePool(p); +#endif +} + void convert_efi_path(char16_t *path) { assert(path); diff --git a/src/boot/util.h b/src/boot/util.h index e1d65770d6..323fb5163b 100644 --- a/src/boot/util.h +++ b/src/boot/util.h @@ -14,18 +14,9 @@ /* This is provided by the linker. */ extern uint8_t __executable_start[]; -static inline void free(void *p) { - if (!p) - return; - - /* Debugging an invalid free requires trace logging to find the call site or a debugger attached. For - * release builds it is not worth the bother to even warn when we cannot even print a call stack. */ -#ifdef EFI_DEBUG - assert_se(BS->FreePool(p) == EFI_SUCCESS); -#else - (void) BS->FreePool(p); -#endif -} +DISABLE_WARNING_REDUNDANT_DECLS; +void free(void *p); +REENABLE_WARNING; static inline void freep(void *p) { free(*(void **) p); diff --git a/src/fundamental/iovec-util-fundamental.h b/src/fundamental/iovec-util-fundamental.h index 6b658c8958..4c86197e6f 100644 --- a/src/fundamental/iovec-util-fundamental.h +++ b/src/fundamental/iovec-util-fundamental.h @@ -17,7 +17,7 @@ struct iovec { }; DISABLE_WARNING_REDUNDANT_DECLS; -static inline void free(void *p); +void free(void *p); REENABLE_WARNING; #endif From 84104ad613c5035ed7d3811d97fbb15362b94f7f Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Sun, 20 Apr 2025 12:25:42 +0200 Subject: [PATCH 2/7] libsystemd: Skip _sd-common.h include check when __clang_analyzer__ is defined This check doesn't work properly when tools such as clang-tidy are analyzing headers. Since we don't care about the check in that case, skip it if __clang_analyzer__ is defined similar to how we skip it as well if __COVERITY__ is defined. --- src/systemd/_sd-common.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/systemd/_sd-common.h b/src/systemd/_sd-common.h index 5792dd8106..00537eaf16 100644 --- a/src/systemd/_sd-common.h +++ b/src/systemd/_sd-common.h @@ -19,7 +19,7 @@ /* This is a private header; never even think of including this directly! */ -#if defined(__INCLUDE_LEVEL__) && __INCLUDE_LEVEL__ <= 1 && !defined(__COVERITY__) +#if defined(__INCLUDE_LEVEL__) && __INCLUDE_LEVEL__ <= 1 && !defined(__COVERITY__) && !defined(__clang_analyzer__) # error "Do not include _sd-common.h directly; it is a private header." #endif From a843382d1e478fcadfdb6d017424ac165a09a324 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Sun, 20 Apr 2025 12:26:53 +0200 Subject: [PATCH 3/7] journal: Always compile journal authentication related files Tooling such as clang-tidy is bad at dealing with condition compilation on the build system level instead of at the source file level. What happens? It still tries to analyze the file and fails horribly if the required headers aren't available. Let's work around the issue and make things more consistent at the same time by doing the necessary HAVE_GCRYPT checks inside of the source files instead of doing them at the build system level. We also add some typedefs to allow getting rid of various HAVE_GCRYPT checks. --- src/basic/gcrypt-util.h | 20 ++++++------- src/journal/journalctl-authenticate.c | 6 ++++ src/journal/journalctl-authenticate.h | 12 -------- src/journal/meson.build | 5 +--- src/libsystemd/meson.build | 9 ++---- src/libsystemd/sd-journal/fsprg.c | 30 +++++++++++++++++++ .../sd-journal/journal-authenticate.c | 22 ++++++++++++++ src/libsystemd/sd-journal/journal-file.c | 2 -- src/libsystemd/sd-journal/journal-file.h | 7 +---- 9 files changed, 72 insertions(+), 41 deletions(-) diff --git a/src/basic/gcrypt-util.h b/src/basic/gcrypt-util.h index 2f9c35d15d..5fc176cb6c 100644 --- a/src/basic/gcrypt-util.h +++ b/src/basic/gcrypt-util.h @@ -6,12 +6,13 @@ #include #include +#include "memory-util.h" + #if HAVE_GCRYPT #include #include "dlfcn-util.h" #include "macro.h" -#include "memory-util.h" extern DLSYM_PROTOTYPE(gcry_md_close); extern DLSYM_PROTOTYPE(gcry_md_copy); @@ -46,15 +47,6 @@ extern DLSYM_PROTOTYPE(gcry_strerror); int initialize_libgcrypt(bool secmem); -static inline gcry_md_hd_t* sym_gcry_md_closep(gcry_md_hd_t *md) { - if (!md || !*md) - return NULL; - sym_gcry_md_close(*md); - - return NULL; -} -DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(gcry_md_hd_t, gcry_md_close, NULL); - /* Copied from gcry_md_putc from gcrypt.h due to the need to call the sym_ variant */ #define sym_gcry_md_putc(h,c) \ do { \ @@ -63,4 +55,12 @@ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(gcry_md_hd_t, gcry_md_close, NULL); sym_gcry_md_write((h__), NULL, 0); \ (h__)->buf[(h__)->bufpos++] = (c) & 0xff; \ } while(false) +#else +typedef struct gcry_md_handle *gcry_md_hd_t; + +static inline void sym_gcry_md_close(gcry_md_hd_t h) { + assert(h == NULL); +} #endif + +DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(gcry_md_hd_t, sym_gcry_md_close, NULL); diff --git a/src/journal/journalctl-authenticate.c b/src/journal/journalctl-authenticate.c index d62d887b0e..667c4f4a14 100644 --- a/src/journal/journalctl-authenticate.c +++ b/src/journal/journalctl-authenticate.c @@ -23,6 +23,7 @@ #include "terminal-util.h" #include "tmpfile-util.h" +#if HAVE_GCRYPT static int format_key( const void *seed, size_t seed_size, @@ -51,8 +52,10 @@ static int format_key( return memstream_finalize(&m, ret, NULL); } +#endif int action_setup_keys(void) { +#if HAVE_GCRYPT _cleanup_(unlink_and_freep) char *tmpfile = NULL; _cleanup_close_ int fd = -EBADF; _cleanup_free_ char *path = NULL; @@ -236,4 +239,7 @@ int action_setup_keys(void) { #endif return 0; +#else + return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "Forward-secure sealing not available."); +#endif } diff --git a/src/journal/journalctl-authenticate.h b/src/journal/journalctl-authenticate.h index 2a8ebd55f7..afb6de6e11 100644 --- a/src/journal/journalctl-authenticate.h +++ b/src/journal/journalctl-authenticate.h @@ -1,16 +1,4 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once -#if HAVE_GCRYPT - int action_setup_keys(void); - -#else - -#include "log.h" - -static inline int action_setup_keys(void) { - return log_error_errno(SYNTHETIC_ERRNO(EOPNOTSUPP), "Forward-secure sealing not available."); -} - -#endif diff --git a/src/journal/meson.build b/src/journal/meson.build index b7925fb97f..60cde31c86 100644 --- a/src/journal/meson.build +++ b/src/journal/meson.build @@ -32,6 +32,7 @@ libjournal_core = static_library( journalctl_sources = files( 'journalctl.c', + 'journalctl-authenticate.c', 'journalctl-catalog.c', 'journalctl-filter.c', 'journalctl-misc.c', @@ -40,10 +41,6 @@ journalctl_sources = files( 'journalctl-varlink.c', ) -if conf.get('HAVE_GCRYPT') == 1 - journalctl_sources += files('journalctl-authenticate.c') -endif - if get_option('link-journalctl-shared') journalctl_link_with = [libshared] else diff --git a/src/libsystemd/meson.build b/src/libsystemd/meson.build index fa28aa09fb..0eb9392d5d 100644 --- a/src/libsystemd/meson.build +++ b/src/libsystemd/meson.build @@ -3,6 +3,8 @@ sd_journal_sources = files( 'sd-journal/audit-type.c', 'sd-journal/catalog.c', + 'sd-journal/fsprg.c', + 'sd-journal/journal-authenticate.c', 'sd-journal/journal-file.c', 'sd-journal/journal-send.c', 'sd-journal/journal-vacuum.c', @@ -12,13 +14,6 @@ sd_journal_sources = files( 'sd-journal/sd-journal.c', ) -if conf.get('HAVE_GCRYPT') == 1 - sd_journal_sources += files( - 'sd-journal/fsprg.c', - 'sd-journal/journal-authenticate.c', - ) -endif - audit_type_includes = [config_h, missing_audit_h, 'linux/audit.h'] diff --git a/src/libsystemd/sd-journal/fsprg.c b/src/libsystemd/sd-journal/fsprg.c index 85632b06d3..707177b9f4 100644 --- a/src/libsystemd/sd-journal/fsprg.c +++ b/src/libsystemd/sd-journal/fsprg.c @@ -26,6 +26,8 @@ #define RND_GEN_Q 0x02 #define RND_GEN_X 0x03 +#if HAVE_GCRYPT + #pragma GCC diagnostic ignored "-Wpointer-arith" /* TODO: remove void* arithmetic and this work-around */ @@ -201,6 +203,8 @@ static void CRT_compose(gcry_mpi_t *x, const gcry_mpi_t xp, const gcry_mpi_t xq, /******************************************************************************/ +#endif + size_t FSPRG_mskinbytes(unsigned _secpar) { VALIDATE_SECPAR(_secpar); return 2 + 2 * (_secpar / 2) / 8; /* to store header,p,q */ @@ -216,6 +220,7 @@ size_t FSPRG_stateinbytes(unsigned _secpar) { return 2 + 2 * _secpar / 8 + 8; /* to store header,n,x,epoch */ } +#if HAVE_GCRYPT static void store_secpar(void *buf, uint16_t secpar) { secpar = secpar / 16 - 1; ((uint8_t*) buf)[0] = (secpar >> 8) & 0xff; @@ -229,8 +234,10 @@ static uint16_t read_secpar(const void *buf) { (uint16_t)(((uint8_t*) buf)[1]) << 0; return 16 * (secpar + 1); } +#endif int FSPRG_GenMK(void *msk, void *mpk, const void *seed, size_t seedlen, unsigned _secpar) { +#if HAVE_GCRYPT uint8_t iseed[FSPRG_RECOMMENDED_SEEDLEN]; gcry_mpi_t n, p, q; uint16_t secpar; @@ -273,9 +280,13 @@ int FSPRG_GenMK(void *msk, void *mpk, const void *seed, size_t seedlen, unsigned sym_gcry_mpi_release(q); return 0; +#else + return -EOPNOTSUPP; +#endif } int FSPRG_GenState0(void *state, const void *mpk, const void *seed, size_t seedlen) { +#if HAVE_GCRYPT gcry_mpi_t n, x; uint16_t secpar; int r; @@ -296,9 +307,13 @@ int FSPRG_GenState0(void *state, const void *mpk, const void *seed, size_t seedl sym_gcry_mpi_release(x); return 0; +#else + return -EOPNOTSUPP; +#endif } int FSPRG_Evolve(void *state) { +#if HAVE_GCRYPT gcry_mpi_t n, x; uint16_t secpar; uint64_t epoch; @@ -323,15 +338,23 @@ int FSPRG_Evolve(void *state) { sym_gcry_mpi_release(x); return 0; +#else + return -EOPNOTSUPP; +#endif } uint64_t FSPRG_GetEpoch(const void *state) { +#if HAVE_GCRYPT uint16_t secpar; secpar = read_secpar(state + 0); return uint64_import(state + 2 + 2 * secpar / 8, 8); +#else + return -EOPNOTSUPP; +#endif } int FSPRG_Seek(void *state, uint64_t epoch, const void *msk, const void *seed, size_t seedlen) { +#if HAVE_GCRYPT gcry_mpi_t p, q, n, x, xp, xq, kp, kq, xm; uint16_t secpar; int r; @@ -374,9 +397,13 @@ int FSPRG_Seek(void *state, uint64_t epoch, const void *msk, const void *seed, s sym_gcry_mpi_release(xm); return 0; +#else + return -EOPNOTSUPP; +#endif } int FSPRG_GetKey(const void *state, void *key, size_t keylen, uint32_t idx) { +#if HAVE_GCRYPT uint16_t secpar; int r; @@ -388,4 +415,7 @@ int FSPRG_GetKey(const void *state, void *key, size_t keylen, uint32_t idx) { det_randomize(key, keylen, state + 2, 2 * secpar / 8 + 8, idx); return 0; +#else + return -EOPNOTSUPP; +#endif } diff --git a/src/libsystemd/sd-journal/journal-authenticate.c b/src/libsystemd/sd-journal/journal-authenticate.c index 35412fe39a..9ad9ce3ef3 100644 --- a/src/libsystemd/sd-journal/journal-authenticate.c +++ b/src/libsystemd/sd-journal/journal-authenticate.c @@ -25,6 +25,7 @@ static void* fssheader_free(FSSHeader *p) { DEFINE_TRIVIAL_CLEANUP_FUNC(FSSHeader*, fssheader_free); +#if HAVE_GCRYPT static uint64_t journal_file_tag_seqnum(JournalFile *f) { uint64_t r; @@ -35,8 +36,10 @@ static uint64_t journal_file_tag_seqnum(JournalFile *f) { return r; } +#endif int journal_file_append_tag(JournalFile *f) { +#if HAVE_GCRYPT Object *o; uint64_t p; int r; @@ -76,9 +79,13 @@ int journal_file_append_tag(JournalFile *f) { f->hmac_running = false; return 0; +#else + return -EOPNOTSUPP; +#endif } int journal_file_hmac_start(JournalFile *f) { +#if HAVE_GCRYPT uint8_t key[256 / 8]; /* Let's pass 256 bit from FSPRG to HMAC */ gcry_error_t err; int r; @@ -107,6 +114,9 @@ int journal_file_hmac_start(JournalFile *f) { f->hmac_running = true; return 0; +#else + return -EOPNOTSUPP; +#endif } static int journal_file_get_epoch(JournalFile *f, uint64_t realtime, uint64_t *epoch) { @@ -252,6 +262,7 @@ int journal_file_maybe_append_tag(JournalFile *f, uint64_t realtime) { } int journal_file_hmac_put_object(JournalFile *f, ObjectType type, Object *o, uint64_t p) { +#if HAVE_GCRYPT int r; assert(f); @@ -307,9 +318,13 @@ int journal_file_hmac_put_object(JournalFile *f, ObjectType type, Object *o, uin } return 0; +#else + return -EOPNOTSUPP; +#endif } int journal_file_hmac_put_header(JournalFile *f) { +#if HAVE_GCRYPT int r; assert(f); @@ -334,6 +349,9 @@ int journal_file_hmac_put_header(JournalFile *f) { sym_gcry_md_write(f->hmac, &f->header->data_hash_table_offset, offsetof(Header, tail_object_offset) - offsetof(Header, data_hash_table_offset)); return 0; +#else + return -EOPNOTSUPP; +#endif } int journal_file_fss_load(JournalFile *f) { @@ -415,6 +433,7 @@ int journal_file_fss_load(JournalFile *f) { } int journal_file_hmac_setup(JournalFile *f) { +#if HAVE_GCRYPT gcry_error_t e; int r; @@ -430,6 +449,9 @@ int journal_file_hmac_setup(JournalFile *f) { return -EOPNOTSUPP; return 0; +#else + return -EOPNOTSUPP; +#endif } int journal_file_append_first_tag(JournalFile *f) { diff --git a/src/libsystemd/sd-journal/journal-file.c b/src/libsystemd/sd-journal/journal-file.c index 0420314894..c4e2bd1591 100644 --- a/src/libsystemd/sd-journal/journal-file.c +++ b/src/libsystemd/sd-journal/journal-file.c @@ -302,7 +302,6 @@ JournalFile* journal_file_close(JournalFile *f) { free(f->compress_buffer); #endif -#if HAVE_GCRYPT if (f->fss_file) { size_t sz = PAGE_ALIGN(f->fss_file_size); assert(sz < SIZE_MAX); @@ -314,7 +313,6 @@ JournalFile* journal_file_close(JournalFile *f) { if (f->hmac) sym_gcry_md_close(f->hmac); -#endif return mfree(f); } diff --git a/src/libsystemd/sd-journal/journal-file.h b/src/libsystemd/sd-journal/journal-file.h index 35f8f6ce6b..6019681316 100644 --- a/src/libsystemd/sd-journal/journal-file.h +++ b/src/libsystemd/sd-journal/journal-file.h @@ -4,14 +4,11 @@ #include #include -#if HAVE_GCRYPT -# include -#endif - #include "sd-event.h" #include "sd-id128.h" #include "compress.h" +#include "gcrypt-util.h" #include "hashmap.h" #include "journal-def.h" #include "missing_fcntl.h" @@ -106,7 +103,6 @@ typedef struct JournalFile { void *compress_buffer; #endif -#if HAVE_GCRYPT gcry_md_hd_t hmac; bool hmac_running; @@ -121,7 +117,6 @@ typedef struct JournalFile { void *fsprg_seed; size_t fsprg_seed_size; -#endif /* When we insert this file into the per-boot priority queue 'newest_by_boot_id' in sd_journal, then by these keys */ sd_id128_t newest_boot_id; From 5332be60d3897c7b86d28cf7b9d61c5dc6847fd6 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 23 Apr 2025 11:45:59 +0200 Subject: [PATCH 4/7] tree-wide: Use assert_se() instead of assert() in various places assert() is compiled away if NDEBUG is set which causes an unused variable warning in various places when the next commit is applied so let's use assert_se() to avoid these warnings. --- src/libsystemd-network/sd-ndisc-router-solicit.c | 6 +++--- src/login/logind-dbus.c | 4 ++-- src/shared/tpm2-util.c | 2 +- src/test/test-label.c | 4 ++-- src/test/test-terminal-util.c | 14 +++++++------- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/libsystemd-network/sd-ndisc-router-solicit.c b/src/libsystemd-network/sd-ndisc-router-solicit.c index 04e7c26c34..ec662accee 100644 --- a/src/libsystemd-network/sd-ndisc-router-solicit.c +++ b/src/libsystemd-network/sd-ndisc-router-solicit.c @@ -49,9 +49,9 @@ int ndisc_router_solicit_parse(sd_radv *ra, sd_ndisc_router_solicit *rs) { "Too small to be a router solicit, ignoring."); const struct nd_router_solicit *a = (const struct nd_router_solicit*) rs->packet->raw_packet; - assert(a); - assert(a->nd_rs_type == ND_ROUTER_SOLICIT); - assert(a->nd_rs_code == 0); + assert_se(a); + assert_se(a->nd_rs_type == ND_ROUTER_SOLICIT); + assert_se(a->nd_rs_code == 0); r = ndisc_parse_options(rs->packet, &rs->options); if (r < 0) diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index c6eac67c0a..926843e868 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -2280,7 +2280,7 @@ static int verify_shutdown_creds( * to catch. In any case, it also means that the payload guarded by * these polkit calls should never be executed, and hence we should * never reach this point. */ - assert(!error_or_denial); + assert_se(!error_or_denial); return 0; } @@ -3841,7 +3841,7 @@ static int method_inhibit(sd_bus_message *message, void *userdata, sd_bus_error * to catch. In any case, it also means that the payload guarded by * these polkit calls should never be executed, and hence we should * never reach this point. */ - assert(!error_or_denial); + assert_se(!error_or_denial); r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_EUID|SD_BUS_CREDS_PID|SD_BUS_CREDS_PIDFD, &creds); if (r < 0) diff --git a/src/shared/tpm2-util.c b/src/shared/tpm2-util.c index 6c2ec26a69..e9b8d6cad4 100644 --- a/src/shared/tpm2-util.c +++ b/src/shared/tpm2-util.c @@ -4897,7 +4897,7 @@ static int tpm2_kdfe( void *end = mempcpy(mempcpy(stpcpy(info, label) + 1, context_u, context_u_size), context_v, context_v_size); /* assert we copied exactly the right amount that we allocated */ - assert(end > info && (uintptr_t) end - (uintptr_t) info == info_len); + assert_se(end > info && (uintptr_t) end - (uintptr_t) info == info_len); _cleanup_free_ void *buf = NULL; r = kdf_ss_derive( diff --git a/src/test/test-label.c b/src/test/test-label.c index cac6cc0f50..12b8a6d748 100644 --- a/src/test/test-label.c +++ b/src/test/test-label.c @@ -139,12 +139,12 @@ TEST(label_ops_post) { fd = RET_NERRNO(get_dir_fd("label_test_dir", 0755)); text1 = "Add initial texts to file for testing label operations to file1\n"; - assert(labelling_op(fd, text1, "file1.txt", 0644) == 0); + assert_se(labelling_op(fd, text1, "file1.txt", 0644) == 0); assert_se(label_ops_post(fd, "file1.txt", true) == 0); assert_se(strlen(text1) == (size_t)buf.st_size); text2 = "Add text2 data to file2\n"; - assert(labelling_op(fd, text2, "file2.txt", 0644) == 0); + assert_se(labelling_op(fd, text2, "file2.txt", 0644) == 0); assert_se(label_ops_post(fd, "file2.txt", true) == 0); assert_se(strlen(text2) == (size_t)buf.st_size); assert_se(label_ops_post(fd, "file3.txt", true) == -ENOENT); diff --git a/src/test/test-terminal-util.c b/src/test/test-terminal-util.c index a5616c8121..907a6c5c73 100644 --- a/src/test/test-terminal-util.c +++ b/src/test/test-terminal-util.c @@ -300,19 +300,19 @@ TEST(pty_open_peer) { _cleanup_free_ char *pty_path = NULL; pty_fd = openpt_allocate(O_RDWR|O_NOCTTY|O_CLOEXEC|O_NONBLOCK, &pty_path); - assert(pty_fd >= 0); - assert(pty_path); + assert_se(pty_fd >= 0); + assert_se(pty_path); peer_fd = pty_open_peer(pty_fd, O_RDWR|O_NOCTTY|O_CLOEXEC); - assert(peer_fd >= 0); + assert_se(peer_fd >= 0); static const char x[] = { 'x', '\n' }; - assert(write(pty_fd, x, sizeof(x)) == 2); + assert_se(write(pty_fd, x, sizeof(x)) == 2); char buf[3]; - assert(read(peer_fd, &buf, sizeof(buf)) == sizeof(x)); - assert(buf[0] == x[0]); - assert(buf[1] == x[1]); + assert_se(read(peer_fd, &buf, sizeof(buf)) == sizeof(x)); + assert_se(buf[0] == x[0]); + assert_se(buf[1] == x[1]); } TEST(terminal_new_session) { From d25c238f97529942b71680c67b759b32005a3951 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Sun, 20 Apr 2025 21:20:42 +0200 Subject: [PATCH 5/7] basic: Move various macros from assert-util.h to assert-fundamental.h ASSERT_PTR() and friends in assert-fundamental.h make use of assert() and assert_se() which when not building for sd-boot are defined in assert-util.h. Because assert() from glibc is only overridden in assert-util.h, the macros in assert-fundamental.h still end up using the glibc assert. Let's fix this by moving the required macros and related logic to assert-fundamental.h. --- src/basic/assert-util.h | 54 -------------------------- src/basic/macro.h | 3 -- src/boot/meson.build | 1 + src/fundamental/assert-fundamental.h | 57 ++++++++++++++++++++++++++++ src/fundamental/macro-fundamental.h | 3 ++ 5 files changed, 61 insertions(+), 57 deletions(-) diff --git a/src/basic/assert-util.h b/src/basic/assert-util.h index 9383a6bb60..7462e1b222 100644 --- a/src/basic/assert-util.h +++ b/src/basic/assert-util.h @@ -9,66 +9,12 @@ void log_set_assert_return_is_critical(bool b); bool log_get_assert_return_is_critical(void) _pure_; -_noreturn_ void log_assert_failed(const char *text, const char *file, int line, const char *func); -_noreturn_ void log_assert_failed_unreachable(const char *file, int line, const char *func); void log_assert_failed_return(const char *text, const char *file, int line, const char *func); -#ifdef __COVERITY__ - -/* Use special definitions of assertion macros in order to prevent - * false positives of ASSERT_SIDE_EFFECT on Coverity static analyzer - * for uses of assert_se() and assert_return(). - * - * These definitions make expression go through a (trivial) function - * call to ensure they are not discarded. Also use ! or !! to ensure - * the boolean expressions are seen as such. - * - * This technique has been described and recommended in: - * https://community.synopsys.com/s/question/0D534000046Yuzb/suppressing-assertsideeffect-for-functions-that-allow-for-sideeffects - */ - -extern void __coverity_panic__(void); - -static inline void __coverity_check__(int condition) { - if (!condition) - __coverity_panic__(); -} - -static inline int __coverity_check_and_return__(int condition) { - return condition; -} - -#define assert_message_se(expr, message) __coverity_check__(!!(expr)) - -#define assert_log(expr, message) __coverity_check_and_return__(!!(expr)) - -#else /* ! __COVERITY__ */ - -#define assert_message_se(expr, message) \ - do { \ - if (_unlikely_(!(expr))) \ - log_assert_failed(message, PROJECT_FILE, __LINE__, __func__); \ - } while (false) - #define assert_log(expr, message) ((_likely_(expr)) \ ? (true) \ : (log_assert_failed_return(message, PROJECT_FILE, __LINE__, __func__), false)) -#endif /* __COVERITY__ */ - -#define assert_se(expr) assert_message_se(expr, #expr) - -/* We override the glibc assert() here. */ -#undef assert -#ifdef NDEBUG -#define assert(expr) ({ if (!(expr)) __builtin_unreachable(); }) -#else -#define assert(expr) assert_message_se(expr, #expr) -#endif - -#define assert_not_reached() \ - log_assert_failed_unreachable(PROJECT_FILE, __LINE__, __func__) - #define assert_return(expr, r) \ do { \ if (!assert_log(expr, #expr)) \ diff --git a/src/basic/macro.h b/src/basic/macro.h index a022e60e17..4ee1ad4d57 100644 --- a/src/basic/macro.h +++ b/src/basic/macro.h @@ -42,9 +42,6 @@ #error "neither int nor long are four bytes long?!?" #endif -assert_cc(STRLEN(__FILE__) > STRLEN(RELATIVE_SOURCE_PATH) + 1); -#define PROJECT_FILE (&__FILE__[STRLEN(RELATIVE_SOURCE_PATH) + 1]) - static inline uint64_t u64_multiply_safe(uint64_t a, uint64_t b) { if (_unlikely_(a != 0 && b > (UINT64_MAX / a))) return 0; /* overflow */ diff --git a/src/boot/meson.build b/src/boot/meson.build index 52f6350620..e393ed9d74 100644 --- a/src/boot/meson.build +++ b/src/boot/meson.build @@ -92,6 +92,7 @@ endforeach efi_conf.set_quoted('PROJECT_VERSION', project_major_version) efi_conf.set_quoted('VERSION_TAG', version_tag) efi_conf.set('PROJECT_URL', conf.get('PROJECT_URL')) +efi_conf.set_quoted('RELATIVE_SOURCE_PATH', relative_source_path) if meson.is_cross_build() and get_option('sbat-distro') == 'auto' warning('Auto detection of SBAT information not supported when cross-building, disabling SBAT.') diff --git a/src/fundamental/assert-fundamental.h b/src/fundamental/assert-fundamental.h index 292656aa6b..0b5c7f21de 100644 --- a/src/fundamental/assert-fundamental.h +++ b/src/fundamental/assert-fundamental.h @@ -18,6 +18,63 @@ #define assert_not_reached() efi_assert("Code should not be reached", __FILE__, __LINE__, __func__) #endif #define assert_se(expr) ({ _likely_(expr) ? VOID_0 : efi_assert(#expr, __FILE__, __LINE__, __func__); }) +#else + +_noreturn_ void log_assert_failed(const char *text, const char *file, int line, const char *func); +_noreturn_ void log_assert_failed_unreachable(const char *file, int line, const char *func); + +#ifdef __COVERITY__ + +/* Use special definitions of assertion macros in order to prevent + * false positives of ASSERT_SIDE_EFFECT on Coverity static analyzer + * for uses of assert_se() and assert_return(). + * + * These definitions make expression go through a (trivial) function + * call to ensure they are not discarded. Also use ! or !! to ensure + * the boolean expressions are seen as such. + * + * This technique has been described and recommended in: + * https://community.synopsys.com/s/question/0D534000046Yuzb/suppressing-assertsideeffect-for-functions-that-allow-for-sideeffects + */ + +extern void __coverity_panic__(void); + +static inline void __coverity_check__(int condition) { + if (!condition) + __coverity_panic__(); +} + +static inline int __coverity_check_and_return__(int condition) { + return condition; +} + +#define assert_message_se(expr, message) __coverity_check__(!!(expr)) + +#define assert_log(expr, message) __coverity_check_and_return__(!!(expr)) + +#else /* ! __COVERITY__ */ + +#define assert_message_se(expr, message) \ + do { \ + if (_unlikely_(!(expr))) \ + log_assert_failed(message, PROJECT_FILE, __LINE__, __func__); \ + } while (false) + +#endif /* __COVERITY__ */ + +#define assert_se(expr) assert_message_se(expr, #expr) + +/* We override the glibc assert() here. */ +#undef assert +#ifdef NDEBUG +#define assert(expr) ({ if (!(expr)) __builtin_unreachable(); }) +#else +#define assert(expr) assert_message_se(expr, #expr) +#endif + +#define assert_not_reached() \ + log_assert_failed_unreachable(PROJECT_FILE, __LINE__, __func__) + #endif /* This passes the argument through after (if asserts are enabled) checking that it is not null. */ diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index cab8368c87..dbf0a7d846 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -463,3 +463,6 @@ assert_cc(sizeof(dummy_t) == 0); #define PTR_TO_SIZE(p) ((size_t) ((uintptr_t) (p))) #define SIZE_TO_PTR(u) ((void *) ((uintptr_t) (u))) + +assert_cc(STRLEN(__FILE__) > STRLEN(RELATIVE_SOURCE_PATH) + 1); +#define PROJECT_FILE (&__FILE__[STRLEN(RELATIVE_SOURCE_PATH) + 1]) From e3a70ca482f2ab9a1980fca4412b01189d5ecea0 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 23 Apr 2025 12:05:54 +0200 Subject: [PATCH 6/7] core: Drop unused forward declaration We already include unit.h. --- src/core/manager.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/manager.h b/src/core/manager.h index cbad223d1b..c116c53e91 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -27,7 +27,6 @@ #include "unit.h" struct libmnt_monitor; -typedef struct Unit Unit; /* Enforce upper limit how many names we allow */ #define MANAGER_MAX_NAMES 131072 /* 128K */ From d55c89690759e2e420da6834dece6274b35d23ad Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 23 Apr 2025 12:06:16 +0200 Subject: [PATCH 7/7] core: Merge forward declaration and definition of DynamicUser --- src/core/dynamic-user.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/core/dynamic-user.h b/src/core/dynamic-user.h index 3e0074defb..fe190cb864 100644 --- a/src/core/dynamic-user.h +++ b/src/core/dynamic-user.h @@ -8,19 +8,12 @@ #include "fdset.h" #include "memory-util.h" -typedef struct DynamicUser DynamicUser; typedef struct Manager Manager; -typedef struct DynamicCreds { - /* A combination of a dynamic user and group */ - DynamicUser *user; - DynamicUser *group; -} DynamicCreds; - /* Note that this object always allocates a pair of user and group under the same name, even if one of them isn't * used. This means, if you want to allocate a group and user pair, and they might have two different names, then you * need to allocated two of these objects. DynamicCreds below makes that easy. */ -struct DynamicUser { +typedef struct DynamicUser { Manager *manager; unsigned n_ref; @@ -29,7 +22,13 @@ struct DynamicUser { int storage_socket[2]; char name[]; -}; +} DynamicUser; + +typedef struct DynamicCreds { + /* A combination of a dynamic user and group */ + DynamicUser *user; + DynamicUser *group; +} DynamicCreds; int dynamic_user_serialize(Manager *m, FILE *f, FDSet *fds); int dynamic_user_serialize_one(DynamicUser *d, const char *key, FILE *f, FDSet *fds);