In general we almost never hit those asserts in production code, so users see
them very rarely, if ever. But either way, we just need something that users
can pass to the developers.
We have quite a few of those asserts, and some have fairly nice messages, but
many are like "WTF?" or "???" or "unexpected something". The error that is
printed includes the file location, and function name. In almost all functions
there's at most one assert, so the function name alone is enough to identify
the failure for a developer. So we don't get much extra from the message, and
we might just as well drop them.
Dropping them makes our code a tiny bit smaller, and most importantly, improves
development experience by making it easy to insert such an assert in the code
without thinking how to phrase the argument.
UIDs don't work well over ssh, but locally or with containers they are OK.
In particular, user@.service uses UIDs as identifiers, and it's nice to be
able to copy&paste that UID for interaction with the user's managers.
We recently started making more use of malloc_usable_size() and rely on
it (see the string_erase() story). Given that we don't really support
sytems where malloc_usable_size() cannot be trusted beyond statistics
anyway, let's go fully in and rework GREEDY_REALLOC() on top of it:
instead of passing around and maintaining the currenly allocated size
everywhere, let's just derive it automatically from
malloc_usable_size().
I am mostly after this for the simplicity this brings. It also brings
minor efficiency improvements I guess, but things become so much nicer
to look at if we can avoid these allocation size variables everywhere.
Note that the malloc_usable_size() man page says relying on it wasn't
"good programming practice", but I think it does this for reasons that
don't apply here: the greedy realloc logic specifically doesn't rely on
the returned extra size, beyond the fact that it is equal or larger than
what was requested.
(This commit was supposed to be a quick patch btw, but apparently we use
the greedy realloc stuff quite a bit across the codebase, so this ends
up touching *a*lot* of code.)
We didn't document this behaviour one way or another, so I think it's
OK to change. All callers do the NULL check before callling this to avoid
the assert warning, so it seems reasonable to do it internally.
sd_bus_can_send() is similar, but there we expressly say that an
error is returned on NULL, so I didn't change it.
While sd-bus already provides sd_bus_call() for calling a method
from a complete bus message object, We don't have an equivalent
function for replying from a method with a complete bus message
object.
Currently, we use sd_bus_send(call->bus, m, NULL) instead. Let's
add a shorthand for this pattern and name it sd_bus_reply().
This is unfortunately harder to implement than it sounds. The user's bus
is bound a to the user's lifecycle after all (i.e. only exists as long
as the user has at least one PAM session), and the path dynamically (at
least theoretically, in practice it's going to be the same always)
generated via $XDG_RUNTIME_DIR in /run/.
To fix this properly, we'll thus go through PAM before connecting to a
user bus. Which is hard since we cannot just link against libpam in the
container, since the container might have been compiled entirely
differently. So our way out is to use systemd-run from outside, which
invokes a transient unit that does PAM from outside, doing so via D-Bus.
Inside the transient unit we then invoke systemd-stdio-bridge which
forwards D-Bus from the user bus to us. The systemd-stdio-bridge makes
up the PAM session and thus we can sure tht the bus exists at least as
long as the bus connection is kept.
Or so say this differently: if you use "systemctl -M lennart@foobar"
now, the bus connection works like this:
1. sd-bus on the host forks off:
systemd-run -M foobar -PGq --wait -pUser=lennart -pPAMName=login systemd-stdio-bridge
2. systemd-run gets a connection to the "foobar" container's
system bus, and invokes the "systemd-stdio-bridge" binary as
transient service inside a PAM session for the user "lennart"
3. The systemd-stdio-bridge then proxies our D-Bus traffic to
the user bus.
sd-bus (on host) → systemd-run (on host) → systemd-stdio-bridge (in container)
Complicated? Well, to some point yes, but otoh it's actually nice in
various other ways, primarily as it makes the -H and -M codepaths more
alike. In the -H case (i.e. connect to remote host via SSH) a very
similar three steps are used. The only difference is that instead of
"systemd-run" the "ssh" binary is used to invoke the stdio bridge in a
PAM session of some other system. Thus we get similar implementation and
isolation for similar operations.
Fixes: #14580
Let's clean up hostname_is_valid() a bit: let's turn the second boolean
argument into a more explanatory flags field, and add a flag that
accepts the special name ".host" as valid. This is useful for the
container logic, where the special hostname ".host" refers to the "root
container", i.e. the host system itself, and can be specified at various
places.
let's also get rid of machine_name_is_valid(). It was just an alias,
which is confusing and even more so now that we have the flags param.
This might fix#17025:
> the call trace is
> bus_ensure_running -> sd_bus_process -> bus_process_internal -> process_closeing --> sd_bus_close
> |
> \-> process_match
We ended doing callouts to the Disconnected matches from bus_ensure_running()
and shouldn't. bus_ensure_running() should never do callouts. This change
should fix this however: once we notice that the connection is going down we
will now fail instantly with ENOTOCONN instead of calling any callbacks.
We would return ENOENT, which is extremely confusing. Strace is not helpful because
no *file* is actually missing. So let's add some logs at debug level and also use
a custom return code. Let all user-facing utilities print a custom error message
in that case.
When connection to the bus fails it can be mighty hard to figure out
what went wrong because we have many different connection mechanisms and
we don't log what is happenning.
By settings AI_ADDRCONFIG in hints we cannot for example resolve "localhost"
when the local machine only has a loopback interface. This seems like an
unnecessary restriction, drop it.
Inspired by https://bugzilla.redhat.com/show_bug.cgi?id=1839007.
poll() sets POLLNVAL inside of the poll structures if an invalid fd is
passed. So far we generally didn't check for that, thus not taking
notice of the error. Given that this specific kind of error is generally
indication of a programming error, and given that our code is embedded
into our projects via NSS or because people link against our library,
let's explicitly check for this and convert it to EBADF.
(I ran into a busy loop because of this missing check when some of my
test code accidentally closed an fd it shouldn't close, so this is a
real thing)
Each of bus_set_address_{user,system} had two users, and each of the two users
would set the internal flag manually. We should do that internally in the
functions instead.
While at it, only set the flag when setting the address is actually successful.
This doesn't change anything for current users, but it seems more correct.
In those functions where bus defaults to the m->bus, we should also
resolve the magic parameters. And if neither called with bus=NULL
and an unattached message, return properly instead of crashing in assert
later.
When authorizing via PolicyKit we want to process incoming method calls
twice: once to process and figure out that we need PK authentication,
and a second time after we aquired PK authentication to actually execute
the operation. With this new call sd_bus_enqueue_for_read() we have a
way to put an incoming message back into the read queue for this
purpose.
This might have other uses too, for example debugging.
This tweaks match installation a bit: the match callbacks are now only
called for messages read after the AddMatch() reply was received and
never anything already read before. Thus, installing a match gives you a
time guarantee: only messages received after it will be matched.
This is useful when listening to PropertiesChanged signals as an example
to ensure that only changes after the point the match was installed are
honoured, nothing before.
If AddMatch() doesn't work, let's destroy the slot for it too as soon as
we received the failure for it.
This way the mere existance of the slot tells us whether the AddMatch()
method call is still pending or is complete.
Let's count incoming messages and attach the current counter when we
first read them to the message objects. This allows us to nicely order
messages later on.
We had atomic counters, but all other operations were non-serialized. This
means that concurrent access to the bus object was only safe if _all_ threads
were doing read-only access. Even sending of messages from threads would not be
possible, because after sending of the message we usually want to remove it
from the send queue in the bus object, which would race. Let's just kill this.
