THis doesn't change the condition's logic at all, but is an attempt to
make things a bit more readable: instead of checking log_target !=
LOG_TARGET_AUTO let's actually list the targets where we want to
consider journal/syslog/kmsg, to make things a bit less confusing. After
all the message here is not to avoid them if LOG_TARGET_AUTO is set, but
to definitely do them in the other cases.
It always was the intention to expose this as trusted field _TID=, i.e.
automatically determine it from journald via some SCM_xyz field or so,
but this is never happened, and it's unlikely this will be added anytime
soon to the kernel either, hence let's just generate this sender side,
even if it means it's untrusted.
This was needed before commit 16e4fd87c5 added a
mode that opens the log fds for every single log message. This mode is used in
execute.c since then making the explicit call to log_open unnecessary.
This basically reverts ea89a119cd.
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.
In recent systemd-nspawn we wouldn't parse init args like systemd.log-level=debug.
This is because we wouldn't even look at /proc/1/cmdline.
$ systemd-nspawn -n cat /proc/1/stat
1 (cat) R 0 1 1 34816 ....
^^^^^
34816 is 136:0 a.k.a. /dev/pts/0.
And obviously CONFIG_LINE=0 is also not logged.
The way that log_syntax_internal now looks is becoming a bit crazy, but we
can't easily conditionalize on both unit and config_file, and we have different
types, so it's not easy to make this more compact.
We would log "(null):0: Failed to parse system call, ignoring: rseq" from
log_syntax_internal() from log_syntax() from seccomp_parse_syscall_filter_full()
from seccomp_parse_syscall_filter() from config_parse_syscall_filter(),
when generating the built-in @default whitelist. Since it was not based on the
unit file, we would not pass a file name.
So let's make sure that log_syntax() does not print "(null)" pointer (which is
iffy and ugly), and use the unit name as fallback or nothing if both are missing.
In principle, one of the two should be always available, since why use log_syntax()
otherwise, but let's make things more resilient by guarding against this case too.
log_syntax() is called from a thousand places, and often in error path, so it's
hard to verify all callers.
These lines are generally out-of-date, incomplete and unnecessary. With
SPDX and git repository much more accurate and fine grained information
about licensing and authorship is available, hence let's drop the
per-file copyright notice. Of course, removing copyright lines of others
is problematic, hence this commit only removes my own lines and leaves
all others untouched. It might be nicer if sooner or later those could
go away too, making git the only and accurate source of authorship
information.
This part of the copyright blurb stems from the GPL use recommendations:
https://www.gnu.org/licenses/gpl-howto.en.html
The concept appears to originate in times where version control was per
file, instead of per tree, and was a way to glue the files together.
Ultimately, we nowadays don't live in that world anymore, and this
information is entirely useless anyway, as people are very welcome to
copy these files into any projects they like, and they shouldn't have to
change bits that are part of our copyright header for that.
hence, let's just get rid of this old cruft, and shorten our codebase a
bit.
If we log to the pty that is configured as stdin/stdout/stderr of the
container too early we risk filling it up in full before we start
processing the pty from the parent process, resulting in deadlocks.
Let's hence keep a copy of the original tty we were started on before
setting up stdin/stdout/stderr, so that we can log to it, and keep using
it as long as we can.
Since the kernel's pty internal buffer is pretty small this actually
triggered deadlocks when we debug logged at lot from nspawn's child
processes, see: https://github.com/systemd/systemd/pull/9024#issuecomment-390403674
With this change we won't use the pty at all, only the actual payload we
start will, and hence we won't deadlock on it, ever.
This extends the change done in b29f6480ec to other logging functions.
This actually fixes some bugs in callers of log_struct(), for example
config_parse_alias() called 'return log_syntax(..., 0, ...)' which could result
in a bogus non-zero return value.
Calls to log_object() and log_format_iovec() — which is only used by
server_driver_message() — appear correct.
Previously we were a bit sloppy with the index and size types of arrays,
we'd regularly use unsigned. While I don't think this ever resulted in
real issues I think we should be more careful there and follow a
stricter regime: unless there's a strong reason not to use size_t for
array sizes and indexes, size_t it should be. Any allocations we do
ultimately will use size_t anyway, and converting forth and back between
unsigned and size_t will always be a source of problems.
Note that on 32bit machines "unsigned" and "size_t" are equivalent, and
on 64bit machines our arrays shouldn't grow that large anyway, and if
they do we have a problem, however that kind of overly large allocation
we have protections for usually, but for overflows we do not have that
so much, hence let's add it.
So yeah, it's a story of the current code being already "good enough",
but I think some extra type hygiene is better.
This patch tries to be comprehensive, but it probably isn't and I missed
a few cases. But I guess we can cover that later as we notice it. Among
smaller fixes, this changes:
1. strv_length()' return type becomes size_t
2. the unit file changes array size becomes size_t
3. DNS answer and query array sizes become size_t
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=76745
Quoting https://github.com/systemd/systemd/pull/8760#discussion_r183321060:
> When we originally added the errno patching we went for a "best of both
> worlds" approach, i.e. that we override errno if an error is specified, but
> if no error is specified (i.e. 0 is passed as error code) then we use the
> previously set errno, similar in style how plain `printf()` would do it. In
> retrospect I think we almost never purposefully made use of the second,
> i.e. the plain `printf()` logic, but we multiple times ran into this case
> accidentally and introduced a bug. Hence yes, it probably makes sense to
> switch this over, and consistently ignore the `errno` already set and always
> override it with the error passed in. The only problem I see with that is: I
> wonder if there might be a case or two lurking somewhere where we actually
> made use of the "best of both worlds" approach, and if so, if we can detect
> where... (But then again, even if there is, and we fail to find those cases,
> maybe that's not all bad, as it's just a few new bugs against probably fixing
> many more old and future bugs, if you follow what I mean).
I scanned our codebase, and found some bugs in the value passed to log_*_errno,
but no intentional cases of error=0 being passed.
Files which are installed as-is (any .service and other unit files, .conf
files, .policy files, etc), are left as is. My assumption is that SPDX
identifiers are not yet that well known, so it's better to retain the
extended header to avoid any doubt.
I also kept any copyright lines. We can probably remove them, but it'd nice to
obtain explicit acks from all involved authors before doing that.
"noreturn" is reserved and can be used in other header files we include:
[ 16s] In file included from /usr/include/gcrypt.h:30:0,
[ 16s] from ../src/journal/journal-file.h:26,
[ 16s] from ../src/journal/journal-vacuum.c:31:
[ 16s] /usr/include/gpg-error.h:1544:46: error: expected ‘,’ or ‘;’ before ‘)’ token
[ 16s] void gpgrt_log_bug (const char *fmt, ...) GPGRT_ATTR_NR_PRINTF(1,2);
Here we include grcrypt.h (which in turns include gpg-error.h) *after* we
"noreturn" was defined in macro.h.
At various places we only want to close fds if they are not
stdin/stdout/stderr, i.e. fds 0, 1, 2. Let's add a unified helper call
for that, and port everything over.
Then it can be used in the asserts in logging functions without causing
infinite recursion. The error is just printed to stderr, it should be
good enough for the common case.
If log_do_header() was called with overly long parameters, it'd generate
improper output. Essentially, it'd be truncated at random point, in particular
missing a newline at the end, so it'd run with the next field, usually MESSAGE=.
log_do_header is called with parameters from compiled code (file name, lien
nubmer, etc), so in practice this was unlikely to ever be a problem, but it is
possible. In particular, if systemd was compiled from sources in some deeply
nested directory (which happens for example in mock and other build roots), the
filename could be very long.
As a safety measure, let's truncate all parameters to 256 bytes. So we have
5 fields which are 256 bytes (plus the field name prefix), and a few other
fields with fixed width. This must always fit in the 2048 byte buffer.
I don't think there's much gain in calculating the required length precisely,
since it's a lot of fields and a few bytes allocated on the stack don't matter.
log_dispatch_internal has only one caller where the extra_field/extra
params are not null: log_unit_full. When log_unit_full() was called,
when we got to log_dispatch_internal, our header would look like this:
PRIORITY=7
SYSLOG_FACILITY=3
CODE_FILE=../src/core/manager.c
CODE_LINE=2145
CODE_FUNC=manager_invoke_sigchld_event
USER_UNIT=gnome-terminal-server.service
65dffa7a3b984a6d9a46f0b8fb57710bUSER_INVOCATION_ID=
SYSLOG_IDENTIFIER=systemd
It took me a while to understand why I'm not seeing mangled messages in the
journal (after all, "" is a valid rvalue for log messages). The answer is that
journald rejects any field name which starts with a digit, and the MESSAGE_ID
that was used here starts with a digit. Hence, those lines would be silently
filtered out.
Zbigniew Jędrzejewski-Szmek