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()`).
- 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 -- \
--- /dev/null
+/* 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
+ ...
+}
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
musl-clang
musl-gcc
ruff
+ coccinelle
shellcheck
PrepareScripts=%D/mkosi/mkosi.images/build/mkosi.conf.d/opensuse/mkosi.prepare
Packages=
clang-tools
+ coccinelle
gh
lcov
libtss2-tcti-device0
usec_t usa = 0, usb = 0;
UnitTimes *times;
+ assert(a);
+ assert(b);
+
times = hashmap_get(unit_times_hashmap, *a);
if (times)
usa = times->activated;
}
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;
}
static void set_destroy_ignore_pointer_max(Set **s) {
+ assert(s);
+
if (*s == POINTER_MAX)
return;
set_free(*s);
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))
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"))
EFI_STATUS err;
assert(root_dir);
+ assert(config);
*config = (Config) {
.editor = true,
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;
#define VIEWPORT_RATIO 10
static void event_closep(EFI_EVENT *event) {
+ assert(event);
+
if (!*event)
return;
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;
/* 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
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
EFI_STATUS err;
assert(root);
+ assert(ret);
/* Opens a file, and then verifies it is actually a directory */
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;
}
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 (;;) {
}
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) {
size_t ident;
char *v;
+ assert(var);
+
ident = strlen(name) + 1; /* name + "=" */
if (len < ident)
char *resolved = NULL;
int r;
+ assert(p);
+
if (!*p)
return 0;
static void remove_and_erasep(const char **p) {
int r;
+ assert(p);
+
if (!*p)
return;
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); })
}
static void unset_statp(struct stat **p) {
+ assert(p);
+
if (!*p)
return;
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;
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,
}
static void gnutls_x509_crt_deinitp(gnutls_x509_crt_t *p) {
+ assert(p);
+
gnutls_x509_crt_deinit(*p);
}
};
int c;
+ assert(syspath);
+ assert(subsystem);
+
while ((c = getopt_long(argc, argv, "p:s:dhVm", options, NULL)) >= 0)
switch (c) {
case 'p':
char *_uid = NULL;
const char *_machine = NULL;
+ assert(uid);
+ assert(machine);
+
if (spec) {
const char *at;
}
static void clear_candidate_hashmapp(Manager **m) {
+ assert(m);
+
if (*m)
hashmap_clear((*m)->monitored_mem_pressure_cgroup_contexts_candidates);
}
}
static int compare_metadata(PortableMetadata *const *x, PortableMetadata *const *y) {
+ assert(x);
+ assert(y);
+
return strcmp((*x)->name, (*y)->name);
}
PortableMetadata *item;
size_t k = 0;
+ assert(ret);
+
sorted = new(PortableMetadata*, hashmap_size(unit_files));
if (!sorted)
return -ENOMEM;
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
static int maybe_reload(sd_bus **bus) {
int r;
+ assert(bus);
+
if (!arg_reload)
return 0;
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
*
}
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),
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));
}
static int nvme_port_report(NvmePort *port, bool *plymouth_done) {
+ POINTER_MAY_BE_NULL(plymouth_done);
+
if (!port)
return 0;
}
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);
}
}
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)
}
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();
int r;
assert(c);
+ assert(invalid_config);
STRV_FOREACH(arg, args) {
if (arg_inline) {
char *next;
int32_t val;
+ assert(val_out);
+
if (!current)
return NULL;
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;
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;
assert(line);
assert(*line);
assert(ret_key);
+ assert(ret_attr);
assert(ret_op);
assert(ret_value);
assert(ret_is_case_insensitive);
uint64_t u;
int r;
+ assert(size);
+
r = parse_size(t, 1024, &u);
if (r < 0)
return r;
void *userdata) {
assert(lvalue);
+ assert(ret_func);
+ assert(ret_ltype);
+ assert(ret_data);
/* Ignore any keys with [] as those are translations. */
if (strchr(lvalue, '[')) {
--- /dev/null
+#!/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
# 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')
projects / thirdparty / systemd.git / commitdiff
