LLVM 13 introduced `-Wunused-but-set-variable` diagnostic flag, which
trips over some intentionally set-but-not-used variables or variables
attached to cleanup handlers with side effects (`_cleanup_umask_`,
`_cleanup_(notify_on_cleanup)`, `_cleanup_(restore_sigsetp)`, etc.):
```
../src/basic/process-util.c:1257:46: error: variable 'saved_ssp' set but not used [-Werror,-Wunused-but-set-variable]
_cleanup_(restore_sigsetp) sigset_t *saved_ssp = NULL;
^
1 error generated.
```
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.
We want user records to be extensible, hence we shouldn't complain about
fields we can't parse. In particular we want them to be extensible for
our own future extensions.
Some code already turned the permissive flag when parsing the JSON data,
but most did not. Fix that. A few select cases remain where the bit is
not set: where we just gnerated the JSON data ourselves, and thus can be
reasonably sure that if we can't parse it it's our immediate programming
error and not just us processing a user record from some other tool or a
newer version of ourselves.
So far we basically had two ways to iterate through NSS records: one via
the varlink IPC and one via the userdb.[ch] infra, with slightly
different implementations.
Let's clean this up, and always use userdb.[ch] also when resolving via
userdbd. The different codepaths for the NameServiceSwitch and the
Multiplexer varlink service now differ only in the different flags
passed to the userdb lookup.
Behaviour shouldn't change by this. This is mostly refactoring, reducing
redundant codepaths.
Let's use "exclude" for flags that really exclude records from our
lookup. Let's use "avoid" referring to concepts that when flag is set
we'll not use but we have a fallback path for that should yield the same
result. Let' use "suppress" for suppressing partial info, even if we
return the record otherwise.
So far we used "avoid" for all these cases, which was confusing.
Whiel we are at it, let's reassign the bits a bit, leaving some space
for bits follow-up commits are going to add.
userdbd listens on "two" sockets, that are actually the same: one is a
real AF_UNIX socket in the fs, and the other is a symlink to it.
So far, when userdbd was started from the command line it would make one
a symlink and the other a real socket, but when invoked via unit files
they'd be swapped, i.e. the other would be a symlink and the one a real
socket.
Let's bring this in line.
Since the "io.systemd.Multiplexer" is our main interface, let's make it
the one exposed as socket, and then make "io.systemd.NameServiceSwitch"
a symlink to it. Or in other words, let's adjust the C code to match the
unit file.
As suggested in https://github.com/systemd/systemd/pull/11484#issuecomment-775288617.
This does not touch anything exposed in src/systemd. Changing the defines there
would be a compatibility break.
Note that tests are broken after this commit. They will be fixed in the next one.
I think this formatting was originally used because it simplified
adding new options to the help messages. However, these days, most
tools their help message end with "\nSee the %s for details.\n" so
the final line almost never has to be edited which eliminates the
benefit of the custom formatting used for printf() help messages.
Let's make things more consistent and use the same formatting for
printf() help messages that we use everywhere else.
Prompted by https://github.com/systemd/systemd/pull/18355#discussion_r567241580
... when called with a valid environment variable name. This means that
any time we call it with a fixed string, it is guaranteed to return 0.
(Also when the variable is not present in the environment block.)
They both are both short and contain similar parts and various helper will be
shared between both parts of the code so it's easier to use a single file.
User records have the realname/gecos fields, groups never had that, but
it would really be useful to have it, hence let's add it with similar
semantics.
We enforce the same syntax as for GECOS, since it's better to start with
strict rules and losen them later instead of the opposite.
This is mostly cosmetic, but let's reorder the destructors so that
we do the final sd_notify() call before we run the destructor for
the manager object.
Presently, CLI utilities such as systemctl will check whether they have a tty
attached or not to decide whether to parse /proc/cmdline or EFI variable
SystemdOptions looking for systemd.log_* entries.
But this check will be misleading if these tools are being launched by a
daemon, such as a monitoring daemon or automation service that runs in
background.
Make log handling of CLI tools uniform by never checking /proc/cmdline or EFI
variables to determine the logging level.
Furthermore, introduce a new log_setup_cli() shortcut to set up common options
used by most command-line utilities.
Previously we'd used the existance of a specific AF_UNIX socket in the
abstract namespace as lock for disabling lookup recursions. (for
breaking out of the loop: userdb synthesized from nss → nss synthesized
from userdb → userdb synthesized from nss → …)
I did it like that because it promised to work the same both in static
and in dynmically linked environments and is accessible easily from any
programming language.
However, it has a weakness regarding reuse attacks: the socket is
securely hashed (siphash) from the thread ID in combination with the
AT_RANDOM secret. Thus it should not be guessable from an attacker in
advance. That's only true if a thread takes the lock only once and
keeps it forever. However, if a thread takes and releases it multiple
times an attacker might monitor that and quickly take the lock
after the first iteration for follow-up iterations.
It's not a big issue given that userdb (as the primary user for this)
never released the lock and we never made the concept a public
interface, and it was only included in one release so far, but it's
something that deserves fixing. (moreover it's a local DoS only, only
permitting to disable native userdb lookups)
With this rework the libnss_systemd.so.2 module will now export two
additional symbols. These symbols are not used by glibc, but can be used
by arbitrary programs: one can be used to disable nss-systemd, the other
to check if it is currently disabled.
The lock is per-thread. It's slightly less pretty, since it requires
people to manually link against C code via dlopen()/dlsym(), but it
should work safely without the aforementioned weakness.
Prompted by the discussion on #16110, let's migrate more code to
fd_wait_for_event().
This only leaves 7 places where we call into poll()/poll() directly in
our entire codebase. (one of which is fd_wait_for_event() itself)
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)
The userdb_by_name() invocation immediately following does the same check
anyway, no need to do this twice.
(Also, make sure we exit the function early on failure)
It fully initializes the address structure, so no need for pre-initialization,
and also returns the length of the address, so no need to recalculate using
SOCKADDR_UN_LEN().
socklen_t is unsigned, so let's not use an int for it. (It doesn't matter, but
seems cleaner and more portable to not assume anything about the type.)
The enum used for column names is integer type while table_set_display() is parsing
arguments on size_t alignment which may result in assert in table_set_display() if
the size between types missmatch. This patch cast the enums to size_t.
It also fixes all other occurences for table_set_display() and
table_set_sort().
