Our coccinelle/check-pointer-deref.cocci checker has a special
case for `assert(pidref_is_set(param))`. It turns out we can
generalize this and catch the following:
- iovec_is_set
- sd_dhcp_duid_is_set
- sd_dhcp_client_id_is_set
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()`).
The comment about `SD_BUS_VTABLE_CAPABILITY(CAP_SYS_BOOT)` is
incorrect. To quote @YHNdnzj:
```
nah, the capability-based permission model is a legacy from kdbus. We cannot do it race-freely without it. Please simply drop the comment.
```
Thanks to @YHNdnzj for suggesting this.
The manager_do_set_objective() was doing a bunch of work to
check the `root` path that can already be done via
`json_dispatch_path` so instead of duplicating use the helper.
Thanks to @YHNdnzj for suggesting this.
Drop the _manager suffix for vl_method_{poweroff,{soft,}reboot,
halt,kexec}_manager.
They're unambiguous enough and the power mgmt operations are
not manager-wide but system-wide.
Thanks to @YHNdnzj for suggesting this.
External tools that use the systemd varlink ecosystem require
to know if a specific varlink method supports/requires the
"more" flag from the IDL. This is tracked upstream in
https://github.com/varlink/varlink.github.io/issues/26
As an intermediate step systemd adds the (very nice) comments
```
# [Requires 'more' flag]
or
# [Supports 'more' flag]
```
to the various methods.
This commit extends the comment around the code that adds the
comment to clarify that this should be considered API and that
the comment should not be changed as external tools (like e.g.
the varlink-http-bridge) rely on it.
This commit allows unset pidref when calling manager_log_caller().
With that we can log manager calls even if we cannot resolve the
caller. Currently when we cannot resolve the caller we are just
not logging anything. With this commit we at least log the call
(even though we don't know what caller it was).
Thanks to keszybz for the suggestion.
Extract a common helper varlink_log_caller() and use in the varlink
code when logging the caller of a method. It also logs the method
now that was tried (but failed) to be logged with log_notice just
like manager_log_caller() would do.
I was looking into modifying `manager_log_caller` instead and
accept a NULL pidref but could not log more than the method without
pidref and would make the manager_log_caller slightly less nice.
Thanks to keszybz for suggesting this.
This adds the low-level io.systemd.Manager shutdown support. This
is (much) simpler than the logind one. It mimics dbus but uses
a shared helper for the simple cases.
Note that this is more restrictive than the dbus version. The
dbus version uses SD_BUS_VTABLE_CAPABILITY(CAP_SYS_BOOT) but
the varlink version uses varlink_check_privileged_peer(link).
This is mostly because I'm not sure how to do the equivalent
in a race-free way.
Thanks to Daan for suggesting this.
This commit extracts the socket forwarding code from the existing
socket-proxyd into a new shared helper that will be used by the
varlinkctl protocol upgrade support code and is used as is in
the socket-proxyd.c.
It tries to keep the changes as small as possible, its mostly
renaming like:
* connection_create_pipes -> socket_forward_create_pipes
* connection_shovel -> socket_forward_shovel
* connection_enable_event_sources -> socket_forward_enable_event_sources
* traffic_cb -> socket_forward_traffic_cb
and a new socket_forward_new() that creates/starts the forwarding.
All log_error_errno() got downgraded to log_debug_errno().
Currently the varlink interface for hostname is only available
after sysinit. This means it is not available until systemd-firstboot
is finished. But there is information like the boot-id in there that
is useful to get early.
My use-case is to query the system early via the varlink-http-bridge
and currently I can't get data from io.systemd.Hostname until
systemd-firstboot is completed which is a bit limiting.
So to fix it this commit sets DefaultDependencies=no on both the socket
and service units.
It also changes hostnamed.c to use
bus_open_system_watch_bind_with_description() which means we will
reconnect once dbus is available. This mimics what resolved-bus.c
is doing (and which was originally introduced in d7afd945b).
Thanks to Lennart for pointing this out.
With the planned extraction of the socket-forward code its useful
to have a basic way to validate the functionality. So add a basic
test that ensures at least base functionality is intact.
In a project I'm working on I recently observed a boot failure
with the most recent version of systemd. It seems it is triggered
by bb19b61049 which fixed a bug that now leads to the function
being excuted differently. The code is missing a check if
`*ret_version` is actually valid in the `ret_measured = true`
case.
We already have some varlink support for the journal to perform
some actions like `Rotate`. It would be nice to be able to query
the journal via varlink too so this commit adds a new varlinkctl
based journal service that exposes a single GetEntries() call
to retrieve journal entries. Basic filtering is supported and
we can expand the API as needed.
This is a separate `io.systemd.JournalControl` [1] service from the
existing `io.systemd.Journald` to decouple read and write (thanks
to Lennart for suggesting this).
This also extracts some shared helper so that we do not duplicate
code when generating the json or when adding the filters.
[1] The name mirrors the bootctl->io.systemd.BootControl naming.
The CleanPool requires --more to be set and checks that in
`vl_method_clean_pool`. By switching to SD_VARLINK_REQUIRES_MORE
this will automatically be handled and is more clear to
the varlink users.
Based on the comment from Lennart in
https://github.com/systemd/systemd/pull/40650#discussion_r2832378002
and the work done by Mike in 09388a6b9e (thanks!).
When sd_varlink_connect_url() gets an unknown URL we now
check if there is a `$LIBEXECDIR/varlink-bridges/$scheme`
binary and execute it (with the url as the first arguments).
This makes varlink more flexible as it provides a way to
dynamically add "bridges" in LIBEXECDIR/varlink-bridges/. This is
conceptually similar to the libvarlink `varlink --bridge` command
and allows to e.g. call varlink over http{,s} via e.g. the new
varlink-http-bridge.
With a running varlink-http-bridge [0] one can do:
```console
$ varlinkctl call http://localhost:8080/ws/sockets/io.systemd.Hostname \
io.systemd.Hostname.Describe {}
{
"Hostname" : "top",
...
```
Closes: https://github.com/systemd/systemd/issues/40640
[0] https://github.com/mvo5/varlink-http-bridge/pull/1
This patch adds a `--ephemeral` option to vmspawn that will turn
on snapshot mode on the qemu disk passed via `--image` or the
directory passed via `--directory`. For disk images it uses the
native mechanism that qemu provides. For directories it (re)uses
the snapshot mechanism that nspawn is using, i.e. it will create
a btrfs snapshot if possible and if not falls back to a traditional
directory copy.
This commit extracts the creation of the snapshot for
nspawn into a shared helper. Its not a lot of code but its
subtle so having a single place seems beneficial.
While extracting the snapshot code got tweaked/simplified based
on the feedback from Yu (many thanks!).
With that we can simplify the snapshot delete and error handling
as well. The shared helper is then going to be used by vmspawn
when it also gets the --ephemeral option.
Michael Vogt