fgetspent_sane() already reports read errors through its return value,
which both loops handle. The leftover check looked at whatever errno the
loop bodies had left behind and could make write_temporary_shadow() fail
with an unrelated error.
Missed in 100d5f6ee6.
Here the logging was fine, but the bogus r == -1 value was propagated
as the return value. The parent then logged
"Remounting '…' failed abnormally, child process … failed: Operation not permitted"
whatever the real error was, and returned -EPERM. Do what
umount_with_timeout() already does with umount2().
Use the option instead of timeout(1), and add test cases for the exit
status on timeout.
This also adds test cases for waiting for a non-existent unit as a
non-root user.
The option takes a time span and limits how long "systemctl wait" waits
for the unit to terminate. If the timeout elapses, the command fails with
exit status 124, which is the exit status used by timeout(1).
The option is currently only used by the wait verb. Other verbs that
wait for units, e.g. "start --wait" or "kill --wait", do not use it yet.
Add a timeout argument to bus_wait_for_units_run(). If the timeout
elapses before the units are ready, the function returns -ETIME.
All callers currently pass USEC_INFINITY, hence there is no functional
change. This is a preparation for the next commit.
Previously, bus_wait_for_units_run() drove the bus connection manually
with sd_bus_process() and sd_bus_wait(). Let's use sd-event instead,
which makes it much easier to add a timeout in a later commit.
Although the man page says
> A unit that does not exist is treated like a unit that terminated
> successfully, and a message about this is shown.
systemctl wait fails as follows when invoked by a non-root user:
```
$ systemctl wait nonexistent.service
Unit nonexistent.service has been removed while waiting for it.
Failed to wait for unit nonexistent.service to terminate.
$ echo $?
1
```
This makes the unit be treated as non-existent when the initial
GetAll() call returns LoadState=not-found. With this change,
```
$ systemctl wait nonexistent.service
Unit nonexistent.service does not exist, treating as terminated.
$ echo $?
0
```
Follow-up for 0e51517319.
If --no-block is specified, the waiter is not allocated. But
bus_wait_for_units_run() was called unconditionally, and it asserts that
its argument is not NULL.
Only wait for the units when the waiter is allocated, as verb_kill()
does.
libcurl regularly asks for very short timeouts, most prominently a
timeout of 0 to kick off a new transfer. We so far armed the curl timer
with the default accuracy of 250ms, which means every such kick-off might
be delayed by up to 250ms.
Rather than disabling the accuracy logic altogether, pick the accuracy
dynamically: if the timeout is closer than the default accuracy, request
maximum accuracy, otherwise go back to the default accuracy, so that
longer curl timeouts can still be coalesced with other wakeups.
To make this possible, export the default accuracy constant of sd-event
as EVENT_DEFAULT_ACCURACY_USEC in event-util.h.
This is based on the analysis and fix by Joe Kurokawa in #44544, which
set the accuracy to 1ms unconditionally. The test case
test_curl_glue_start_latency() is taken from that PR as-is, written by
Joe Kurokawa.
Fixes: #44478
Alternative to: #44544
search_ipc() returns a positive value when IPC objects exist, which is not
an error. Passing it to log_debug_errno() made the message end in
"Operation not permitted".
Running a varlink job via io.systemd.Unit.StartTransient and
watching the progress stops if there is a daemon-reload. This
commit fixes this similar to how D-Bus keeps working on daemon
reloads.
So far catalog_get() opened, fstat()ed, mmap()ed, validated and unmapped
the catalog database on every single lookup. When showing logs we look
up the catalog for every entry that carries a MESSAGE_ID= field, once
for the documentation hyperlink and once more with -x, hence we paid
that price up to twice per entry.
Introduce a CatalogFile object that keeps the database mapped, and
cache it in sd_journal, opened lazily on the first
sd_journal_get_catalog() call and released by sd_journal_close(). If
opening fails (e.g. because no catalog is installed), remember the
error so that we don't retry for every entry either.
catalog_get(), catalog_list() and catalog_list_items() are reworked on
top of the new object. catalog_list_items() now maps the database once
for all requested ids. The fd is closed right after mapping, so that
long-lived library users don't keep an extra fd around.
With this, "journalctl -x -n 2000" opens the catalog database once
instead of 174 times on my system.
Convert every ASSERT_STRV_EQ(…, STRV_MAKE(…)) in the tests. Lists that had
been wrapped only to make room for STRV_MAKE() and now fit on one line are
joined.
The remaining ASSERT_STRV_EQ(…, STRV_MAKE(…)) in test-tests.c are the
smoke tests for that macro and stay.
strjoina() is used inline in a bunch of the tests, as well as other
non-trivial function calls, but that is OK, since the expresion is only
evaluated once by the macro.
The linebreaks in test-env-util.c:replace_env_argv_bad are adjusted so
that input and output have the same structure.
The tree opens files via the xopenat() family, which sets O_CLOEXEC on
its own and refuses to create files through dangling symlinks. Nothing
stops new code from calling open() directly though. The repart manifest
code added such a call after the migration. Let's add open(), openat()
and creat() to the custom functions of bugprone-unsafe-functions so that
clang-tidy reports direct calls.
The functions that implement xopenat() and fd_reopen() call the syscalls
directly, and so do the seccomp tests and the O_EMPTYPATH probe in
test-fd-util.c. We suppress the check in those places.
openat2() isn't on the list. override/fcntl.h defines openat2 as a macro
for openat2_shim(), and the check ignores calls that come from a macro
expansion. An entry for openat2() wouldn't report anything.
The manifest generation from f4bff983b2 ("repart: optionally write
UAPI.16 manifests of populated partitions and of the partition table")
was merged after the move to the xopenat() family. It opens the populated
tree with a direct open() call. Let's use xopenat() like the rest of
repart.
The tree outside the tests opens files via the xopenat() family, but the
tests in src/test/ still call open(), openat() and creat() directly. Let's
port them as well, so that clang-tidy can flag every direct call.
test-fdset.c needs an fd without O_CLOEXEC, so it clears the flag with
fd_cloexec() after opening. The seccomp tests and the O_EMPTYPATH probe in
test-fd-util.c keep calling the syscalls directly, since that's what they
test.
`umount_recursive_full()` breaks out of the mountinfo iteration after
the first successful `umount2()`, then the outer loop re-parses the
whole of `/proc/self/mountinfo`. Tearing down N mounts below a prefix
therefore costs N full table parses.
## Fix
Don't break out. Unmount everything reachable from one snapshot of the
table, and only re-parse if the pass made progress. Anything still busy
(a parent reached before its child) is retried on the next pass.
Since an unmount can invalidate later entries of the snapshot, each path
is now checked against the mount ID from the table before unmounting.
Otherwise, detaching a parent reached before an older child leaves the
child's old path resolving through the uncovered tree, and a symlink
there can lead to a mount outside of the prefix or on the keep list. If
a path does not match or an unmount fails after the pass already
unmounted something, the table is re-parsed first. The new test covers
this and fails without the check.
## Numbers
`umount_recursive_full()` timed with an equal number of unrelated mounts
present (mean of 5 runs, `-O0`):
| mounts below prefix | before | after |
|---|---|---|
| 100 | 20.0 ms | 1.6 ms |
| 300 | 145.1 ms | 4.0 ms |
| 600 | 538.2 ms | 8.0 ms |
## Testing
`test-mount-util` passes in an unprivileged user and mount namespace,
not as root. Not covered: propagation, autofs, and the path where the
`statx()` check itself fails (it now falls through to `umount2()`);
extra eyes there are welcome.
Address Lennart's post-merge review of #43692:
* Check for open errors once in sync-util.
* Use the ERRNO_IS_NEG_*() helpers on returned errors.
* Add is_symlink_at() and propagate errors from the dangling symlink
checks.
* Propagate unexpected F_CREATED_QUERY errors.
* Try MSG_CMSG_CLOEXEC first in recvmsg_safe() and retry without it
instead of checking the socket family.
* Refuse FORK_PACK_FDS without FORK_CLOEXEC_OFF.
* Explain why fd_reopen() tries O_EMPTYPATH before ".".
* Add argument comments and a test for is_symlink_at().
http.server.HTTPServer.server_bind() calls socket.getfqdn(), i.e. resolves the
host name via NSS. On sanitizer builds this loads our ASan-instrumented NSS
modules into the uninstrumented python interpreter, which then aborts with
"ASan runtime does not come first in initial library list", and TEST-25-IMPORT
times out waiting for the server. Use socketserver.TCPServer instead, which
doesn't do the lookup.
If the same bank is specified twice we'd extend the PCR twice in that bank in
one go, and write a userspace measurement log record that lists the same
digest algorithm twice. systemd-pcrlock refuses such records ("Failed to add
bank to event log record: File exists"), which in turn breaks everything that
reads the event log, for example systemd-report-sign-tpm2.
TEST-70-TPM2.pcrextend.sh deliberately calls --bank=sha256 --bank=sha256, and
on systems without a firmware event log (where TEST-70-TPM2.pcrlock.sh is
skipped and hence doesn't reset the log) TEST-70-TPM2.report-sign.sh then fails
because the TPM2 signer gets EEXIST from io.systemd.PCRLock.ReadEventLog.
A shorthand for ASSERT_STRV_EQ(l, STRV_MAKE(…)) which takes the expected
elements directly, like STR_IN_SET() does. When the comparison fails,
the elements are printed as written, followed by both lists, e.g.
Expected "l == ["foo", "baz"]", got ["foo", "bar"] != ["foo", "baz"]
Make that caveat from the previous commit more obvious with a logged
warning, and document the workaround (or really, the fix) of disabling
`net.ipv4.conf.all.rp_filter` globally.
Signed-off-by: Martin Pitt <martin@amutable.com>
Formatting a JSON variant and then writing it to a file via
write_string_file_at() is a common pattern. Let's add a helper that
combines the two, so that callers don't have to deal with the
intermediary string buffer anymore. The buffer is erased after use,
since JSON objects might carry sensitive data.
"systemctl disable" can't turn a unit off that the vendor enabled with a
dependency symlink below /usr/. Where a mask has been created by hand it removes
the mask, which silently enables the unit again. A preset policy of "disable"
can't turn such a unit off either.
/usr/ is frequently read-only and anything we did there would be undone by the
next image update, so let's shadow the symlink instead: disable writes a symlink
to /dev/null into the configuration directory, and PID 1 ignores a dependency
entry that resolves to /dev/null. Only the symlinks that enabling would create
are shadowed, i.e. the ones in the directories the [Install] section names. A
static dependency such as the symlink to remote-cryptsetup.target in
initrd-root-device.target.wants/ stays untouched. Enable removes the masks
again, those of the Also= units included, which is why the removal runs after
the symlinks have been applied. Creating a symlink replaces the mask at the same
path. Disabling a template masks every instance the vendor enabled, and enabling
the template only creates the symlink of its default instance, so the removal
afterwards catches the masks of the other instances.
Disabling a template also removes the symlinks of its instances.
"systemctl disable foo@.service" therefore deletes a foo@bar.service ->
/dev/null unit mask, which unmasks the instance. Disable now leaves all masks
alone, unit masks included.
Static dependencies are left alone throughout, so that a catch-all "disable *"
policy can't stop the likes of systemd-journald.service from starting at boot.
Nothing is masked where something else already exists in the dependency
directory: it already shadows the vendor symlink, and replacing it would destroy
it.
A preset policy of "disable" deliberately doesn't mask this way and leaves the
vendor enablement in place. Preset files never had to name the units the vendor
enabled below /usr/, and with a catch-all "disable *" rule a mask would stop
all of those units from starting at boot.
"systemctl --user disable" warns when a unit stays enabled in global scope, and
checks the global state for that. A vendor symlink in /usr/lib/systemd/user/ now
counts there, even though the mask the disable just created in the user's
configuration shadows it. Let's check the user scope's state instead. It sees
the mask, and it still reports a unit enabled in /etc/systemd/user/ as enabled.
The changes list gains two types for this, "mask-dependency" and
"unmask-dependency", so that callers can tell a mask from a plain unlink. The
bus API documents them as such, GetUnitFileLinks() reports the vendor symlink
rather than the mask that would shadow it, and so does "is-enabled --full".
Distributions install .wants/, .requires/ and .upholds/ symlinks in
/usr/lib/systemd/system/, and PID 1 acts on those exactly like on the ones in
/etc/. The install layer doesn't know about that tier at all, so "systemctl
is-enabled" reports such units as "disabled" (or "static" for instances) even
though they start at boot. On a stock Arch install man-db.timer reports
"disabled" while being active.
Let's teach find_symlinks_in_scope() about that tier. A dependency symlink
below /usr/ now enables the unit, but only one that "systemctl enable" would
create, i.e. one in a directory that WantedBy=, RequiredBy= or UpheldBy= in the
[Install] section names. Any other such symlink is a static dependency rather
than enablement, and reporting the unit as "enabled" because of it would
suggest it can be disabled again. remote-cryptsetup.target has
WantedBy=multi-user.target, and we install a symlink to it in
initrd-root-device.target.wants/, which must not make it report as enabled.
Alias= and Also= don't count, as neither results in a dependency symlink.
This means "systemctl is-enabled" now exits 0 where it used to exit 1 for these
units. Distributions install such symlinks for a number of units, so packaging
scripts may notice. Turning them off is a separate matter: disable still can't,
which is no worse than before.
With `systemd.imds.network=locked`, imdsd cannot reach IMDS at all. Our
sysctl.d/50-default.conf enables IPv4 reverse path filtering for all
links. Locked mode adds a "prohibit" route for the IMDS address to the
main table. imdsd's marked sockets use their own table, so its
_requests_ get out. But IMDS's _replies_ carry no mark: the reverse path
filter checks their source against the main table, finds the prohibit
route, and drops them.
To fix this, turn off reverse path filtering on the configured links in
locked mode. The prohibit route still blocks direct access for everybody
else.
Caveat: This does not help where `net.ipv4.conf.all.rp_filter` is on, as
the kernel applies the maximum of that and the link's setting. Ubuntu
does that by default, so for now this only works out of the box on
Debian, Fedora, etc. Cover that case in the tests, and expect a
timeout/failure with `all.rp_filter > 0`.
Fixes#44602
Signed-off-by: Martin Pitt <martin@amutable.com>
Zbigniew Jędrzejewski-Szmek