From 777d9c10ada1f027e488452b3777e3712f14f0a9 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 13 Mar 2026 14:46:37 +0100 Subject: [PATCH] coccinelle: add checks for pointer access without NULL check The fix in 8f1751a111 made me wonder if we could automatically detect when pointers are accessed but when this might not be safe. Systemd is already using a lot of `assert(dst)` and this change now forces us to use them. So this commit (ab)uses coccinelle to flag any pointer parameter dereference not preceded by assert(param), ASSERT_PTR(param), or an explicit NULL check. It adds integration into meson as a new "coccinelle" test suite (just like clang-tidy) and is run in CI. The check is not perfect but seems a reasonable heuristic. For this RFC commit it is scoped to a subset, it excludes 25 dirs right now and includes around 100. About 300 warnings left. Busywork that I am happy to do if there is agreement that it is worth it. With this in place we would have caught the bug from 8f1751a111 in CI: ``` FAIL: check-pointer-deref.cocci found issues in systemd/src/boot: diff -u -p systemd/src/boot/measure.c /tmp/nothing/measure.c --- systemd/src/boot/measure.c +++ /tmp/nothing/measure.c @@ -312,7 +312,6 @@ EFI_STATUS tpm_log_tagged_event( if (err != EFI_SUCCESS) return err; - *ret_measured = true; return EFI_SUCCESS; } ``` This also adds a new POINTER_MAY_BE_NULL() for the cases when the called function will do the NULL check (like `iovec_is_set()`). --- .github/workflows/linter.yml | 3 ++ coccinelle/check-pointer-deref.cocci | 35 +++++++++++++++++ meson.build | 39 +++++++++++++++++++ .../mkosi.tools.conf/mkosi.conf.d/fedora.conf | 1 + .../mkosi.conf.d/opensuse.conf | 1 + src/analyze/analyze-critical-chain.c | 3 ++ src/analyze/analyze-security.c | 2 + src/analyze/analyze-verify-util.c | 2 + src/analyze/analyze.c | 2 + src/boot/boot.c | 3 ++ src/boot/chid.c | 2 + src/boot/console.c | 5 +++ src/boot/efi-string.c | 2 + src/boot/initrd.c | 1 + src/boot/util.c | 1 + src/bootctl/bootctl-install.c | 2 + src/busctl/busctl.c | 2 + src/cgtop/cgtop.c | 2 +- src/coredump/coredumpctl.c | 4 ++ src/cryptsetup/cryptsetup.c | 2 + src/fundamental/assert-fundamental.h | 5 +++ src/hostname/hostnamed.c | 2 + src/journal-remote/journal-gatewayd.c | 4 +- src/journal-remote/journal-remote-main.c | 2 + src/journal-remote/microhttpd-util.c | 2 + src/libudev/test-libudev.c | 3 ++ src/machine/machinectl.c | 3 ++ src/oom/oomd-manager.c | 2 + src/portable/portable.c | 5 +++ src/portable/portablectl.c | 4 ++ src/repart/repart.c | 6 +++ src/storagetm/storagetm.c | 2 + src/sysext/sysext.c | 3 ++ src/systemctl/systemctl-list-dependencies.c | 3 ++ src/systemctl/systemctl-show.c | 3 ++ src/tmpfiles/tmpfiles.c | 1 + src/udev/udev-builtin-keyboard.c | 2 + src/udev/udev-builtin-path_id.c | 4 ++ src/udev/udev-rules.c | 1 + src/veritysetup/veritysetup.c | 2 + .../xdg-autostart-service.c | 3 ++ tools/check-coccinelle.sh | 28 +++++++++++++ tools/meson.build | 1 + 43 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 coccinelle/check-pointer-deref.cocci create mode 100755 tools/check-coccinelle.sh diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml index ba293cf8be..775b4f3f9d 100644 --- a/.github/workflows/linter.yml +++ b/.github/workflows/linter.yml @@ -80,6 +80,9 @@ jobs: - name: Run clang-tidy run: mkosi box -- meson test -C build --suite=clang-tidy --print-errorlogs --no-stdsplit --quiet + - name: Run coccinelle checks + run: mkosi box -- meson test -C build --suite=coccinelle --print-errorlogs --no-stdsplit + - name: Build with musl run: | mkosi box -- \ diff --git a/coccinelle/check-pointer-deref.cocci b/coccinelle/check-pointer-deref.cocci new file mode 100644 index 0000000000..e2376a314c --- /dev/null +++ b/coccinelle/check-pointer-deref.cocci @@ -0,0 +1,35 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Detect pointer parameters that are dereferenced without a prior NULL check + * or assertion. In systemd style, non-optional pointer parameters should have + * an assert() at the top of the function. + * + * Usage: + * spatch --sp-file coccinelle/check-pointer-deref.cocci --dir src/boot/ + * + * Note: this is a context-mode rule (flags, does not auto-fix). Each flagged + * dereference should be reviewed: if the parameter is never NULL, add + * assert(param) at the top. If it can legitimately be NULL, add an if() guard. + */ +@@ +identifier fn, param; +type T; +position p; +@@ + +fn(..., T *param, ...) { + ... when != assert(param) + when != assert(param != NULL) + when != assert_se(param) + when != assert_se(param != NULL) + when != assert_return(param, ...) + when != ASSERT_PTR(param) + when != POINTER_MAY_BE_NULL(param) + /* NULL-safe helpers used commonly enough in assert() to warrant inclusion + * here. For less common cases, use POINTER_MAY_BE_NULL(param) instead of + * extending this list. */ + when != assert(pidref_is_set(param)) + when != \( param == NULL \| param != NULL \| !param \) +* *param@p + ... +} diff --git a/meson.build b/meson.build index 2893bea332..36025023db 100644 --- a/meson.build +++ b/meson.build @@ -2972,6 +2972,45 @@ if meson.version().version_compare('>=1.4.0') endforeach endif +spatch = find_program('spatch', required : false) +if spatch.found() + # Directories excluded from coccinelle checks until their warnings are fixed. + # Remove directories from this list as they are cleaned up. + coccinelle_exclude = [ + 'src/basic/', + 'src/core/', + 'src/import/', + 'src/journal/', + 'src/libc/', + 'src/libsystemd/', + 'src/libsystemd-network/', + 'src/network/', + 'src/nspawn/', + 'src/nss-systemd/', + 'src/resolve/', + 'src/shared/', + 'src/test/', + ] + + coccinelle_src_dirs = run_command( + 'sh', '-c', 'printf "%s\n" src/*/', + check : true, + ).stdout().strip().split('\n') + + foreach dir : coccinelle_src_dirs + if dir not in coccinelle_exclude + test( + 'coccinelle-@0@'.format(fs.name(dir)), + check_coccinelle_sh, + args : [meson.project_source_root() / dir, + meson.project_source_root() / 'coccinelle'], + suite : 'coccinelle', + timeout : 120, + ) + endif + endforeach +endif + symbol_analysis_exes = [] foreach name, exe : executables_by_name symbol_analysis_exes += exe diff --git a/mkosi/mkosi.tools.conf/mkosi.conf.d/fedora.conf b/mkosi/mkosi.tools.conf/mkosi.conf.d/fedora.conf index 7a9301c566..e687fd788e 100644 --- a/mkosi/mkosi.tools.conf/mkosi.conf.d/fedora.conf +++ b/mkosi/mkosi.tools.conf/mkosi.conf.d/fedora.conf @@ -13,4 +13,5 @@ Packages= musl-clang musl-gcc ruff + coccinelle shellcheck diff --git a/mkosi/mkosi.tools.conf/mkosi.conf.d/opensuse.conf b/mkosi/mkosi.tools.conf/mkosi.conf.d/opensuse.conf index 8a6711f901..6f24649c54 100644 --- a/mkosi/mkosi.tools.conf/mkosi.conf.d/opensuse.conf +++ b/mkosi/mkosi.tools.conf/mkosi.conf.d/opensuse.conf @@ -7,6 +7,7 @@ Distribution=opensuse PrepareScripts=%D/mkosi/mkosi.images/build/mkosi.conf.d/opensuse/mkosi.prepare Packages= clang-tools + coccinelle gh lcov libtss2-tcti-device0 diff --git a/src/analyze/analyze-critical-chain.c b/src/analyze/analyze-critical-chain.c index ea6d83d417..659d1b564c 100644 --- a/src/analyze/analyze-critical-chain.c +++ b/src/analyze/analyze-critical-chain.c @@ -67,6 +67,9 @@ static int list_dependencies_compare(char *const *a, char *const *b) { usec_t usa = 0, usb = 0; UnitTimes *times; + assert(a); + assert(b); + times = hashmap_get(unit_times_hashmap, *a); if (times) usa = times->activated; diff --git a/src/analyze/analyze-security.c b/src/analyze/analyze-security.c index bdbd44910b..5e9db877b7 100644 --- a/src/analyze/analyze-security.c +++ b/src/analyze/analyze-security.c @@ -558,6 +558,8 @@ static int assess_system_call_architectures( } static bool syscall_names_in_filter(Set *s, bool allow_list, const SyscallFilterSet *f, const char **ret_offending_syscall) { + assert(ret_offending_syscall); + NULSTR_FOREACH(syscall, f->value) { if (syscall[0] == '@') { const SyscallFilterSet *g; diff --git a/src/analyze/analyze-verify-util.c b/src/analyze/analyze-verify-util.c index dfa1cf7b3b..e7ffae5a28 100644 --- a/src/analyze/analyze-verify-util.c +++ b/src/analyze/analyze-verify-util.c @@ -268,6 +268,8 @@ static int verify_unit(Unit *u, bool check_man, const char *root) { } static void set_destroy_ignore_pointer_max(Set **s) { + assert(s); + if (*s == POINTER_MAX) return; set_free(*s); diff --git a/src/analyze/analyze.c b/src/analyze/analyze.c index 4d078a4838..e23b0038a9 100644 --- a/src/analyze/analyze.c +++ b/src/analyze/analyze.c @@ -124,6 +124,8 @@ STATIC_DESTRUCTOR_REGISTER(arg_image_policy, image_policy_freep); int acquire_bus(sd_bus **bus, bool *use_full_bus) { int r; + POINTER_MAY_BE_NULL(use_full_bus); + if (use_full_bus && *use_full_bus) { r = bus_connect_transport(arg_transport, arg_host, arg_runtime_scope, bus); if (IN_SET(r, 0, -EHOSTDOWN)) diff --git a/src/boot/boot.c b/src/boot/boot.c index 4a7e616faa..bffedf78e2 100644 --- a/src/boot/boot.c +++ b/src/boot/boot.c @@ -1046,6 +1046,8 @@ static BootEntry* boot_entry_free(BootEntry *entry) { DEFINE_TRIVIAL_CLEANUP_FUNC(BootEntry *, boot_entry_free); static EFI_STATUS config_timeout_sec_from_string(const char *value, uint64_t *dst) { + assert(dst); + if (streq8(value, "menu-disabled")) *dst = TIMEOUT_MENU_DISABLED; else if (streq8(value, "menu-force")) @@ -1555,6 +1557,7 @@ static void config_load_defaults(Config *config, EFI_FILE *root_dir) { EFI_STATUS err; assert(root_dir); + assert(config); *config = (Config) { .editor = true, diff --git a/src/boot/chid.c b/src/boot/chid.c index 28f2b7b898..8abd9de47c 100644 --- a/src/boot/chid.c +++ b/src/boot/chid.c @@ -99,6 +99,8 @@ static EFI_STATUS populate_board_chids(EFI_GUID ret_chids[static CHID_TYPES_MAX] EFI_STATUS chid_match(const void *hwid_buffer, size_t hwid_length, uint32_t match_type, const Device **ret_device) { EFI_STATUS status; + assert(ret_device); + if ((uintptr_t) hwid_buffer % alignof(Device) != 0) return EFI_INVALID_PARAMETER; diff --git a/src/boot/console.c b/src/boot/console.c index 21b36e5a6e..81a5641d40 100644 --- a/src/boot/console.c +++ b/src/boot/console.c @@ -11,6 +11,8 @@ #define VIEWPORT_RATIO 10 static void event_closep(EFI_EVENT *event) { + assert(event); + if (!*event) return; @@ -191,6 +193,9 @@ EFI_STATUS query_screen_resolution(uint32_t *ret_w, uint32_t *ret_h) { EFI_STATUS err; EFI_GRAPHICS_OUTPUT_PROTOCOL *go; + assert(ret_w); + assert(ret_h); + err = BS->LocateProtocol(MAKE_GUID_PTR(EFI_GRAPHICS_OUTPUT_PROTOCOL), NULL, (void **) &go); if (err != EFI_SUCCESS) return err; diff --git a/src/boot/efi-string.c b/src/boot/efi-string.c index 0f8986b598..d0be7a1e16 100644 --- a/src/boot/efi-string.c +++ b/src/boot/efi-string.c @@ -349,6 +349,8 @@ static bool efi_fnmatch_prefix(const char16_t *p, const char16_t *h, const char1 /* Patterns are fnmatch-compatible (with reduced feature support). */ bool efi_fnmatch(const char16_t *pattern, const char16_t *haystack) { + assert(haystack); + /* Patterns can be considered as simple patterns (without '*') concatenated by '*'. By doing so we * simply have to make sure the very first simple pattern matches the start of haystack. Then we just * look for the remaining simple patterns *somewhere* within the haystack (in order) as any extra diff --git a/src/boot/initrd.c b/src/boot/initrd.c index d8cbe7deed..b8086ac633 100644 --- a/src/boot/initrd.c +++ b/src/boot/initrd.c @@ -74,6 +74,7 @@ EFI_STATUS initrd_register( EFI_HANDLE handle; struct initrd_loader *loader; + POINTER_MAY_BE_NULL(initrd); assert(ret_initrd_handle); /* If no initrd is specified we'll not install any. This avoids registration of the protocol for that diff --git a/src/boot/util.c b/src/boot/util.c index 4a4c4e9365..c40a9aad65 100644 --- a/src/boot/util.c +++ b/src/boot/util.c @@ -344,6 +344,7 @@ EFI_STATUS open_directory( EFI_STATUS err; assert(root); + assert(ret); /* Opens a file, and then verifies it is actually a directory */ diff --git a/src/bootctl/bootctl-install.c b/src/bootctl/bootctl-install.c index c4693293ab..fc89ce143b 100644 --- a/src/bootctl/bootctl-install.c +++ b/src/bootctl/bootctl-install.c @@ -1230,6 +1230,8 @@ static bool same_entry(uint16_t id, sd_id128_t uuid, const char *path) { static int find_slot(sd_id128_t uuid, const char *path, uint16_t *id) { _cleanup_free_ uint16_t *options = NULL; + assert(id); + int n = efi_get_boot_options(&options); if (n < 0) return n; diff --git a/src/busctl/busctl.c b/src/busctl/busctl.c index 48635fad64..aa04c9bbf0 100644 --- a/src/busctl/busctl.c +++ b/src/busctl/busctl.c @@ -393,6 +393,8 @@ static int verb_list_bus_names(int argc, char *argv[], uintptr_t _data, void *us } static void print_subtree(const char *prefix, const char *path, char **l) { + assert(l); + /* We assume the list is sorted. Let's first skip over the * entry we are looking at. */ for (;;) { diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c index 60181caffc..a9bf64a616 100644 --- a/src/cgtop/cgtop.c +++ b/src/cgtop/cgtop.c @@ -509,7 +509,7 @@ static int refresh( } static int group_compare(Group * const *a, Group * const *b) { - const Group *x = *a, *y = *b; + const Group *x = *ASSERT_PTR(a), *y = *ASSERT_PTR(b); int r; if (arg_order != ORDER_TASKS || arg_recursive) { diff --git a/src/coredump/coredumpctl.c b/src/coredump/coredumpctl.c index f41d6fef31..96724de12b 100644 --- a/src/coredump/coredumpctl.c +++ b/src/coredump/coredumpctl.c @@ -415,6 +415,8 @@ static int retrieve(const void *data, size_t ident; char *v; + assert(var); + ident = strlen(name) + 1; /* name + "=" */ if (len < ident) @@ -529,6 +531,8 @@ static int resolve_filename(const char *root, char **p) { char *resolved = NULL; int r; + assert(p); + if (!*p) return 0; diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 64cd3813b8..04eeb6223e 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -2540,6 +2540,8 @@ static uint32_t determine_flags(void) { static void remove_and_erasep(const char **p) { int r; + assert(p); + if (!*p) return; diff --git a/src/fundamental/assert-fundamental.h b/src/fundamental/assert-fundamental.h index 3168e5699a..e7f662512b 100644 --- a/src/fundamental/assert-fundamental.h +++ b/src/fundamental/assert-fundamental.h @@ -100,3 +100,8 @@ static inline int __coverity_check_and_return__(int condition) { assert_se(_expr_ >= _zero); \ _expr_; \ }) + +/* Mark a pointer parameter as intentionally nullable. This is a no-op at runtime but suppresses + * the coccinelle check-pointer-deref warning for parameters that are safely handled before any + * dereference (e.g. passed to a NULL-safe helper like iovec_is_set()). */ +#define POINTER_MAY_BE_NULL(ptr) ({ (void) (ptr); }) diff --git a/src/hostname/hostnamed.c b/src/hostname/hostnamed.c index dcd1264534..49462c6a65 100644 --- a/src/hostname/hostnamed.c +++ b/src/hostname/hostnamed.c @@ -822,6 +822,8 @@ static int context_update_kernel_hostname( } static void unset_statp(struct stat **p) { + assert(p); + if (!*p) return; diff --git a/src/journal-remote/journal-gatewayd.c b/src/journal-remote/journal-gatewayd.c index ee19082761..c140c406cb 100644 --- a/src/journal-remote/journal-gatewayd.c +++ b/src/journal-remote/journal-gatewayd.c @@ -104,8 +104,10 @@ static void request_meta_free( struct MHD_Connection *connection, void **connection_cls, enum MHD_RequestTerminationCode toe) { + RequestMeta *m; - RequestMeta *m = *connection_cls; + assert(connection_cls); + m = *connection_cls; if (!m) return; diff --git a/src/journal-remote/journal-remote-main.c b/src/journal-remote/journal-remote-main.c index 35ab12578b..0ff44ede6f 100644 --- a/src/journal-remote/journal-remote-main.c +++ b/src/journal-remote/journal-remote-main.c @@ -1103,6 +1103,8 @@ static int parse_argv(int argc, char *argv[]) { static int load_certificates(char **key, char **cert, char **trust) { int r; + assert(trust); + r = read_full_file_full( AT_FDCWD, arg_key ?: PRIV_KEY_FILE, UINT64_MAX, SIZE_MAX, READ_FULL_FILE_SECURE|READ_FULL_FILE_WARN_WORLD_READABLE|READ_FULL_FILE_CONNECT_SOCKET, diff --git a/src/journal-remote/microhttpd-util.c b/src/journal-remote/microhttpd-util.c index 73b6ed4c94..32751e85e1 100644 --- a/src/journal-remote/microhttpd-util.c +++ b/src/journal-remote/microhttpd-util.c @@ -230,6 +230,8 @@ static int get_auth_dn(gnutls_x509_crt_t client_cert, char **buf) { } static void gnutls_x509_crt_deinitp(gnutls_x509_crt_t *p) { + assert(p); + gnutls_x509_crt_deinit(*p); } diff --git a/src/libudev/test-libudev.c b/src/libudev/test-libudev.c index 63c2403124..f15cbc3a91 100644 --- a/src/libudev/test-libudev.c +++ b/src/libudev/test-libudev.c @@ -416,6 +416,9 @@ static int parse_args(int argc, char *argv[], const char **syspath, const char * }; int c; + assert(syspath); + assert(subsystem); + while ((c = getopt_long(argc, argv, "p:s:dhVm", options, NULL)) >= 0) switch (c) { case 'p': diff --git a/src/machine/machinectl.c b/src/machine/machinectl.c index d9ee2fe2bb..733b1a19ef 100644 --- a/src/machine/machinectl.c +++ b/src/machine/machinectl.c @@ -1308,6 +1308,9 @@ static int parse_machine_uid(const char *spec, const char **machine, char **uid) char *_uid = NULL; const char *_machine = NULL; + assert(uid); + assert(machine); + if (spec) { const char *at; diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index 97ad9c0a9f..382a246c2d 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -438,6 +438,8 @@ static int monitor_swap_contexts_handler(sd_event_source *s, uint64_t usec, void } static void clear_candidate_hashmapp(Manager **m) { + assert(m); + if (*m) hashmap_clear((*m)->monitored_mem_pressure_cgroup_contexts_candidates); } diff --git a/src/portable/portable.c b/src/portable/portable.c index a7f3ce9dfd..bae23b1a51 100644 --- a/src/portable/portable.c +++ b/src/portable/portable.c @@ -137,6 +137,9 @@ PortableMetadata *portable_metadata_unref(PortableMetadata *i) { } static int compare_metadata(PortableMetadata *const *x, PortableMetadata *const *y) { + assert(x); + assert(y); + return strcmp((*x)->name, (*y)->name); } @@ -146,6 +149,8 @@ int portable_metadata_hashmap_to_sorted_array(Hashmap *unit_files, PortableMetad PortableMetadata *item; size_t k = 0; + assert(ret); + sorted = new(PortableMetadata*, hashmap_size(unit_files)); if (!sorted) return -ENOMEM; diff --git a/src/portable/portablectl.c b/src/portable/portablectl.c index 98a1657a72..0cd461a997 100644 --- a/src/portable/portablectl.c +++ b/src/portable/portablectl.c @@ -57,6 +57,8 @@ static bool is_portable_managed(const char *unit) { static int determine_image(const char *image, bool permit_non_existing, char **ret) { int r; + assert(ret); + /* If the specified name is a valid image name, we pass it as-is to portabled, which will search for it in the * usual search directories. Otherwise we presume it's a path, and will normalize it on the client's side * (among other things, to make the path independent of the client's working directory) before passing it @@ -235,6 +237,8 @@ static int acquire_bus(sd_bus **bus) { static int maybe_reload(sd_bus **bus) { int r; + assert(bus); + if (!arg_reload) return 0; diff --git a/src/repart/repart.c b/src/repart/repart.c index 1bdeec6ea5..7dbcfd6d58 100644 --- a/src/repart/repart.c +++ b/src/repart/repart.c @@ -652,6 +652,8 @@ static int calculate_verity_hash_size( uint64_t data_block_size, uint64_t *ret_bytes) { + assert(ret_bytes); + /* The calculation here is based on the documented on-disk format of the dm-verity * https://docs.kernel.org/admin-guide/device-mapper/verity.html#hash-tree * @@ -1264,6 +1266,8 @@ static uint64_t free_area_available_for_new_partitions(Context *context, const F } static int free_area_compare(FreeArea *const *a, FreeArea *const*b, Context *context) { + assert(a); + assert(b); assert(context); return CMP(free_area_available_for_new_partitions(context, *a), @@ -4042,6 +4046,8 @@ static void context_unload_partition_table(Context *context) { static int format_size_change(uint64_t from, uint64_t to, char **ret) { char *t; + assert(ret); + if (from != UINT64_MAX) { if (from == to || to == UINT64_MAX) t = strdup(FORMAT_BYTES(from)); diff --git a/src/storagetm/storagetm.c b/src/storagetm/storagetm.c index c6caaa1260..7a4596e472 100644 --- a/src/storagetm/storagetm.c +++ b/src/storagetm/storagetm.c @@ -746,6 +746,8 @@ static int plymouth_notify_port(NvmePort *port, struct local_address *a) { } static int nvme_port_report(NvmePort *port, bool *plymouth_done) { + POINTER_MAY_BE_NULL(plymouth_done); + if (!port) return 0; diff --git a/src/sysext/sysext.c b/src/sysext/sysext.c index 32de5a454c..157bcb0a0b 100644 --- a/src/sysext/sysext.c +++ b/src/sysext/sysext.c @@ -1785,6 +1785,9 @@ static int merge_hierarchy( } static int strverscmp_improvedp(char *const* a, char *const* b) { + assert(a); + assert(b); + /* usable in qsort() for sorting a string array with strverscmp_improved() */ return strverscmp_improved(*a, *b); } diff --git a/src/systemctl/systemctl-list-dependencies.c b/src/systemctl/systemctl-list-dependencies.c index 8e5736ef35..4e7c12e6b9 100644 --- a/src/systemctl/systemctl-list-dependencies.c +++ b/src/systemctl/systemctl-list-dependencies.c @@ -82,6 +82,9 @@ static int list_dependencies_print(const char *name, UnitActiveState state, int } static int list_dependencies_compare(char * const *a, char * const *b) { + assert(a); + assert(b); + if (unit_name_to_type(*a) == UNIT_TARGET && unit_name_to_type(*b) != UNIT_TARGET) return 1; if (unit_name_to_type(*a) != UNIT_TARGET && unit_name_to_type(*b) == UNIT_TARGET) diff --git a/src/systemctl/systemctl-show.c b/src/systemctl/systemctl-show.c index 0872f82c2a..570aab7365 100644 --- a/src/systemctl/systemctl-show.c +++ b/src/systemctl/systemctl-show.c @@ -320,6 +320,9 @@ static void unit_status_info_done(UnitStatusInfo *info) { } static void format_active_state(const char *active_state, const char **active_on, const char **active_off) { + assert(active_on); + assert(active_off); + if (streq_ptr(active_state, "failed")) { *active_on = ansi_highlight_red(); *active_off = ansi_normal(); diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 7885a668fd..17f263790e 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -4433,6 +4433,7 @@ static int parse_arguments( int r; assert(c); + assert(invalid_config); STRV_FOREACH(arg, args) { if (arg_inline) { diff --git a/src/udev/udev-builtin-keyboard.c b/src/udev/udev-builtin-keyboard.c index 5ab40a3552..3ced8ad91c 100644 --- a/src/udev/udev-builtin-keyboard.c +++ b/src/udev/udev-builtin-keyboard.c @@ -90,6 +90,8 @@ static const char* parse_token(const char *current, int32_t *val_out) { char *next; int32_t val; + assert(val_out); + if (!current) return NULL; diff --git a/src/udev/udev-builtin-path_id.c b/src/udev/udev-builtin-path_id.c index cdd8da3203..a252ec99dd 100644 --- a/src/udev/udev-builtin-path_id.c +++ b/src/udev/udev-builtin-path_id.c @@ -398,6 +398,8 @@ static sd_device* handle_scsi_hyperv(sd_device *parent, char **path, size_t guid static sd_device* handle_scsi(sd_device *parent, char **path, char **compat_path, bool *supported_parent) { const char *id, *name; + assert(supported_parent); + if (device_is_devtype(parent, "scsi_device") <= 0) return parent; @@ -454,6 +456,8 @@ static sd_device* handle_cciss(sd_device *parent, char **path) { static void handle_scsi_tape(sd_device *dev, char **path) { const char *name; + assert(path); + /* must be the last device in the syspath */ if (*path) return; diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index f0e4fbccfd..691230d753 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1359,6 +1359,7 @@ static int parse_line(char **line, char **ret_key, char **ret_attr, UdevRuleOper assert(line); assert(*line); assert(ret_key); + assert(ret_attr); assert(ret_op); assert(ret_value); assert(ret_is_case_insensitive); diff --git a/src/veritysetup/veritysetup.c b/src/veritysetup/veritysetup.c index b2f6d3b8af..4a244ae83d 100644 --- a/src/veritysetup/veritysetup.c +++ b/src/veritysetup/veritysetup.c @@ -116,6 +116,8 @@ static int parse_block_size(const char *t, uint64_t *size) { uint64_t u; int r; + assert(size); + r = parse_size(t, 1024, &u); if (r < 0) return r; diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index 62ddce1815..ad77e476c8 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -291,6 +291,9 @@ static int xdg_config_item_table_lookup( void *userdata) { assert(lvalue); + assert(ret_func); + assert(ret_ltype); + assert(ret_data); /* Ignore any keys with [] as those are translations. */ if (strchr(lvalue, '[')) { diff --git a/tools/check-coccinelle.sh b/tools/check-coccinelle.sh new file mode 100755 index 0000000000..c7d1f6f6da --- /dev/null +++ b/tools/check-coccinelle.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -eu +set -o pipefail + +SRC_DIR="${1:?}" +COCCI_DIR="${2:?}" + +FOUND=0 + +for cocci in "$COCCI_DIR"/check-*.cocci; do + [[ -f "$cocci" ]] || continue + output=$(spatch --very-quiet --sp-file "$cocci" --dir "$SRC_DIR" 2>&1) + if [[ -n "$output" ]]; then + echo "FAIL: $(basename "$cocci") found issues in $SRC_DIR:" + echo "$output" + FOUND=1 + fi +done + +if [[ "$FOUND" -ne 0 ]]; then + echo "" + echo "Coccinelle check(s) failed. For each flagged dereference, either:" + echo " - Add assert(param)/ASSERT_PTR(param) at the top of the function (if the parameter must not be NULL)" + echo " - Add an if (param) guard before the dereference (if NULL is valid)" + echo " - Add POINTER_MAY_BE_NULL(param) if NULL is okay for param" + exit 1 +fi diff --git a/tools/meson.build b/tools/meson.build index 3132eeddba..e8b3133d9c 100644 --- a/tools/meson.build +++ b/tools/meson.build @@ -1,6 +1,7 @@ # SPDX-License-Identifier: LGPL-2.1-or-later check_api_docs_sh = files('check-api-docs.sh') +check_coccinelle_sh = files('check-coccinelle.sh') check_efi_alignment_py = files('check-efi-alignment.py') check_help_sh = files('check-help.sh') check_version_history_py = files('check-version-history.py')