Three independent fixes to code on `journalctl`'s per-entry read and
output path. Each of them removes work that was never needed, rather
than trading away anything. There is no behaviour change: the output is
byte for byte identical for every format except `-o json`, where only
the order of the keys within each object can shift, which is already
nondeterministic between two runs of the unpatched binary (see the
correctness section below). Nothing about the formats, tunables or
public API changes.
Measured on a 730 MB journal spread over 100 files (roughly 2M entries):
| command | before | after | |
|---|---:|---:|---:|
| `journalctl -o cat` | 1.93s | 0.77s | **2.51x** |
| `journalctl --grep=<no match>` | 1.97s | 0.82s | **2.40x** |
| `journalctl -o export` | 4.06s | 1.73s | **2.35x** |
| `journalctl -n 100000 -o short` | 0.86s | 0.43s | **2.00x** |
| `journalctl -o json -n 200000` | 5.85s | 3.50s | 1.67x |
| `journalctl -o short` (the default) | 3.32s | 1.96s | **1.69x** |
| `journalctl -o verbose` | 5.99s | 3.61s | 1.66x |
### 1. `utf8_is_printable_newline()` decoded every character twice
`utf8_encoded_valid_unichar()` decodes the character internally in order
to validate it, and then throws the codepoint away. The caller
immediately called `utf8_encoded_to_unichar()` again to get the very
same codepoint back. So for every byte of every field printed, the UTF-8
decoder ran twice, only to answer whether the byte was a control
character.
This is fixed where it originates rather than at the call site:
`utf8_encoded_valid_unichar_full()` optionally returns the character it
had to decode anyway, so a caller that wants both the validity and the
value decodes once. On top of that, eight bytes are tested at once for
plain printable ASCII, which is what almost all journal content is. A
word that does not qualify falls through to the per character path
unchanged, so tabs, newlines and multibyte sequences behave exactly as
before.
On its own, against unmodified main: `-o export` 4.02s to 2.89s.
### 2. sd-journal read the clock once per journal file, per entry
`sd_journal_next()` sweeps every open journal file to find the next
entry, and `next_beyond_location()` rate limits its tail timestamp
refresh per file. Since `ratelimit_below()` reads the clock itself, a
journal spread over a hundred files meant a hundred `clock_gettime()`
calls for every single entry iterated, nearly all of them only to
conclude that the one second interval had not elapsed yet.
This was the single largest cost in `journalctl`. In a profile of `-o
export` it was over a fifth of the entire run, spent deciding not to do
the thing it was guarding.
Add `ratelimit_below_at()`, which takes the timestamp from the caller,
and read the clock once per sweep instead. `ratelimit_below()` keeps its
behaviour and becomes a wrapper.
The timestamp is now sampled once per sweep rather than once per file.
The interval being rate limited is one second and a sweep takes
microseconds, so this makes no practical difference.
On its own: `-o export` 4.02s to 2.93s, and `-o short` 3.30s to 2.14s.
This is the largest single contributor for the default output mode.
### 3. The JSON output built a fresh, unsized hashmap for every entry
`journal_entry_to_json()` creates an empty hashmap per entry and fills
it with that entry's fields, of which there are typically around twenty.
Because it starts empty it has to grow several times per entry, and
every growth rehashes everything inserted so far. `-o json` spent about
a quarter of its time in siphash, a third of that under
`resize_buckets()`.
Reserving room up front is a one line change.
On its own: `-o json -n 200000` 5.71s to 4.64s.
make_console_stdio() forces KD_TEXT while reopening /dev/console. When
systemd-shutdown enters the exitrd, this can expose the text console over
a retained graphical shutdown splash.
Let callers choose whether to switch to text mode, and preserve the mode
for the exitrd handoff. Keep the existing text-mode behavior for manager
reexecution and the crash shell. Restore text mode if executing /shutdown
fails and before reporting a fatal error and freezing.
Add an unsafe, nonparallel virtual-console regression test requiring
explicit opt-in and an already-owned inactive VT in a disposable VM.
Check graphics preservation and text restoration through a private
/dev/console bind mount, retain stderr diagnostics, and restore the saved
mode in the parent even if the child fails.
sd_journal_next() sweeps every open journal file to find the next entry, and
next_beyond_location() rate-limits its tail timestamp refresh per file. Since
ratelimit_below() reads the clock itself, iterating a journal spread over a
hundred files meant a hundred clock_gettime() calls for every single entry,
nearly all of them only to conclude that the one second interval had not
elapsed yet. On a 730M journal in 100 files this was the single largest cost in
journalctl, over a fifth of the run.
Add ratelimit_below_at(), which takes the timestamp from the caller, and read
the clock once per sweep in real_journal_next() instead. ratelimit_below()
keeps its behaviour and becomes a wrapper.
The timestamp is now sampled once for the whole sweep rather than once per
file. The interval being rate-limited is a second and a sweep takes
microseconds, so this makes no practical difference.
On that journal, journalctl -o export goes from 4.02s to 2.93s, and the default
journalctl -o short from 3.30s to 2.14s.
This decoded every character twice: utf8_encoded_valid_unichar() decodes
internally to do its validation and then throws the codepoint away, and the
caller immediately called utf8_encoded_to_unichar() again to get the very same
codepoint back.
Fix that where it originates rather than at the call site: add
utf8_encoded_valid_unichar_full(), which optionally returns the character it had
to decode anyway, and make utf8_encoded_valid_unichar() a wrapper that passes
NULL. Callers that want both the validity and the value now decode once.
On top of that add a bulk fast path, since almost everything we are asked about
is plain printable ASCII: test eight bytes at a time for that, i.e. that no byte
is >= 0x80, none is < 0x20 and none is 0x7f. A remainder of less than eight
bytes is copied into a word padded with spaces so that it can be tested the same
way. A word that does not qualify falls through to the per character path, so
tabs, newlines and multibyte sequences are handled exactly as before.
The function sits on journalctl's per-field output path, so this is measurable:
on a 730M journal spread over 100 files, journalctl -o export goes from 4.02s to
about 2.9s.
Also extend the tests, which so far covered neither DEL nor the C1 range nor
allow_newline=false: place each of the interesting bytes at each offset within
the eight byte word, and check a multibyte character straddling the end of one.
The new code was verified to agree with the old one on 41945090 inputs, for both
values of allow_newline: every byte string of length zero to three over all 256
byte values, four million random strings over uniform bytes, over an ASCII-heavy
alphabet and over an alphabet of encoding boundary bytes, and a corpus placing
every byte value at every offset of buffers of every length from eight to
twenty-four bytes, with valid and truncated multibyte sequences straddling the
word boundary.
This is split out of #43058, but makes sense on its own: and makes
strip_tab_ansi() a lore more complete and correct.
ptybroker is going to use for the ansi sequence parser for other
purposes too, but making strip_tab_ansi() work better is enough of a
reason to have this I think.
strv_rebreak_lines() was the last place left that decoded with the permissive
utf8_encoded_to_unichar() and then asked unichar_console_width() how wide the
result is, so it accepted and measured overlong encodings, surrogates,
noncharacters and the obsolete five and six byte forms that utf8_console_width()
now refuses.
That matters because callers mix the two within a single width computation.
sd-varlink-idl.c sizes its indent with utf8_console_width() and then breaks the
body with strv_rebreak_lines(); verbs.c does the same. One half of the sum
refused a sequence that the other half happily assigned a width to.
Use utf8_char_console_width(), which does the validation and the width in one
step, and advance with utf8_next_char() rather than the decoded length. The
function already refused invalid UTF-8, so the only change is which sequences
count as invalid; it now matches utf8_is_valid(), as everywhere else.
ellipsize_mem() and align_string_mem() open coded the width of a character as
unichar_iswide(c) ? 2 : 1, but unichar_console_width() counts a tab as eight
cells. The two disagreed, and format-table.c uses both: it decides a field is too
wide with utf8_console_width(), then asks ellipsize() to fit it into that many
columns, and ellipsize() believed a tab was one cell. The ellipsized cell then
still overflowed the column, measured by the very function that had just asked
for it to be truncated. A table pinned to table_set_width(20) emitted lines of
25, 44 and 39 columns. align_string_mem() undercounted the same way and so padded
already overlong cells further.
Use unichar_console_width(), split out for this purpose in 5c0d4b5061. While at
it, let align_string_mem() decode with utf8_char_console_width() as well, so that
the second pass shares both the width model and the validity check with the
measuring pass, instead of keeping a more permissive decoder of its own around
for the next caller to trip over. Its "count invalid chars as 1" fallback now
skips the whole rejected sequence rather than one byte, so that the continuation
bytes are not charged a cell each of their own; the branch should be unreachable
anyway, since our caller only gets there after utf8_console_width() succeeded on
the same string.
Note that unichar_console_width() charges a tab a flat eight cells, while a
terminal advances to the next multiple of eight, so this is deliberately an upper
bound: the two agree only when the tab itself starts on a multiple of eight. A
per character helper cannot do better, because a tab's width is a function of the
column it starts in, and neither ellipsize_mem() nor align_string_mem() is told
what column its string starts at. Overestimating is the safe direction: it can
leave a column padded short, but it cannot overflow one.
ascii_ellipsize_mem() equates bytes with character cells, which holds for ASCII
except for the tab, so keep tabs off that fast path. Like the ANSI sequence case
just above it, such strings now take the generic path, which means they get the
Unicode ellipsis and one reserved cell rather than "..." and three, even in a non
UTF-8 locale.
utf8_char_console_width() decoded with utf8_encoded_to_unichar(), which does not
check for overlong encodings, does not check unichar_is_valid(), and still
accepts the obsolete five and six byte forms. Those checks live in the wrapper
utf8_encoded_valid_unichar(), which is what utf8_is_valid() and ellipsize_mem()
use. Over all 4244897280 NUL free byte strings of length one to four, that is the
whole UTF-8 encoding space, the wrapper agrees with utf8_is_valid_n() everywhere,
while the bare decoder disagrees on 8835838 of them, always by accepting more.
So utf8_console_width() assigned a finite width to strings that utf8_is_valid()
refuses. format-table.c measures a cell with utf8_console_width() and, if it does
not fit, hands it to ellipsize(), which validates strictly, returns NULL, and is
reported as -ENOMEM. A surrogate, overlong, noncharacter or five byte sequence in
any cell therefore failed the whole table with a bogus out of memory error, and
only once the column happened to be narrow enough to require truncation, so
whether it happened at all depended on the width of the user's terminal. At other
widths the invalid sequence was printed to the terminal instead.
Decode with utf8_encoded_valid_unichar() so that how wide we think a string is
agrees with whether we consider it valid UTF-8 in the first place. Invalid cells
are now refused consistently with -EINVAL, the way the sequences that
utf8_encoded_to_unichar() already rejected always were.
Note that for tables this makes the refusal total rather than per field:
table_data_requested_width_height() turns a SIZE_MAX width into -EINVAL, which
table_format() propagates, so a cell in one of the newly rejected encodings now
fails the whole table at every width. systemd-dissect and repart --dry-run put
GPT partition names read straight off the image into cells without filtering
them, so a crafted partition name suppresses the whole listing where before it
cost one field, and only at some terminal widths. That is the intended trade:
the alternative is to keep emitting raw invalid bytes to the terminal at
whichever widths happen not to need truncation. Should degrading the cell rather
than the table turn out to be preferable, the place for it is where the text
enters the table, via utf8_escape_invalid() and friends, not in a helper whose
job is to say how wide a string is.
This also makes the assert(w >= 0) in utf8_escape_non_printable_full() hold by
construction rather than by the accident that its own validation was stricter
than the width helper's.
So far strip_tab_ansi() was very superficial: it only focussed on OSC
and CSI sequences, and left the other ANSI sequences in touch.
Let's make it more powerful and port it to the more complete
AnsiSeqParser logic, and strip *all* ANSI sequences.
For the relevant usecases this seems like the better, safer choice,
since it's good to strip too much rather than too little, when
stripping ANSI sequences.
namespace_enter() tried to become root after joining a user namespace
but ignored failures, so if root wasn't mapped we'd just carry on with
our original UID/GID. Let's make that an error instead.
Dropping supplementary groups stays best effort, as setgroups() might
be denied (e.g. by the nsresourced BPF LSM) even when root is mapped.
Some callers do need to enter namespaces without root, so add
NAMESPACE_ENTER_KEEP_UID_GID_IF_ROOT_UNMAPPED for those. It's used by
systemd-analyze unit-shell and by the credential refresh in the user
manager. Everything else enters containers or namespaces we allocated
ourselves, where root is always mapped.
Otherwise, UBSAN complains when src is NULL even though the shift is zero:
../src/basic/compress.c:2074:29: runtime error: applying zero offset to null pointer
dns_packet_append_name() added every name it appends to the packet's
compression map, including names written at offsets >= 0x4000 that an
RFC 1035 compression pointer (14 bits) can never reference. Such
entries are not just useless -- when the same name occurs again, the
lookup finds it, the pointer cannot be emitted, the code falls through
to appending the labels again, and the re-insertion into the map then
fails the whole packet build with hashmap_ensure_put() returning
-EEXIST.
This is easily triggered with mDNS: an unsolicited DNS-SD announcement
covering a zone with a few hundred services exceeds 16KiB, and
dns_scope_announce() then fails with "Failed to build reply packet:
File exists".
This lifts the failure only: dns_scope_announce() still builds the whole
zone into one packet and hands it to the socket as a single datagram,
fragmented as the link requires. Splitting announcements across
MTU-sized packets, as RFC 6762 section 17 asks, is done separately.
Only insert a name into the map if its offset is pointer-expressible,
making "mapped implies referenceable" a map invariant. Re-occurrences of
out-of-range names are emitted as labels again -- the only encoding RFC
1035 section 4.1.4 permits in that case -- still compressed from the
first suffix that is mapped in range.
The lookup restates that range as control flow rather than asserting it.
Under -Db_ndebug=true, which CI builds, assert() is
__builtin_unreachable(), so an out-of-range entry would put 0xC000 | n
truncated to 14 bits on the wire -- a pointer to an arbitrary earlier
offset -- where falling through emits the labels instead. This layer is
shared with unicast DNS, DoT and LLMNR.
The pointer's tag bits and its offset bound are now named once, in
dns-def.h next to the other wire-format constants rather than in
dns-packet.h, since the format is RFC 1035's and not this packet
abstraction's -- dhcp-message.c open-codes the same two bits to parse
compressed names out of DHCP options. Compile-time checks pin that the
two partition the 16-bit field and that the tag is its top two bits,
which is what lets the reader take it from the first octet alone.
The offset's other bound becomes control flow along with it: it was an
assert(), which under -Db_ndebug=true is no check at all, for the bound
whose violation puts a pointer to the wrong bytes on the wire. It is
also tightened to the start of the name being appended rather than the
end of the packet, which is the "prior occurrence" RFC 1035 section
4.1.4 allows a pointer to address and what dns_packet_read_name()
enforces on the way in.
Add regression tests, including one through dns_packet_append_rr() --
the path the reported failure came from, where the name append is nested
inside the rdlength backpatch and two further rollbacks -- and one for
the reader's label-type dispatch, which the renamed constants touch.
decompress_startswith() decodes just enough of a blob to compare its leading
bytes against a prefix, and grows the output buffer whenever a round of
decoding came back with less than prefix_len + 1 bytes decoded. That growth is
only the right answer while the decoder actually wants more output room. If it
instead ran out of input, i.e. the stream is truncated, then no amount of extra
room will help, and the gzip and bzip2 paths double their allocation on every
iteration until the process is out of memory.
Neither decoder lets us tell the two cases apart by return code alone:
inflate() reports the stalled stream as Z_BUF_ERROR, which we cannot simply
refuse since that is also how it asks for a bigger output buffer, and
BZ2_bzDecompress() does not distinguish them at all, it just keeps saying
BZ_OK. The output space left over does tell us, though: a decoder that stopped
without filling the buffer we gave it is waiting for input that is not coming,
so report the stream as corrupt instead of growing.
A 10 byte gzip header, or a 4 byte bzip2 header, with nothing behind it was
enough to reach a single 4 GiB allocation this way. The xz and zstd paths
already terminate, the former because liblzma turns a stalled stream into
LZMA_BUF_ERROR.
Note that no caller can reach this today: the only non-test caller is
decompress_startswith_journal(), and compression_supported_journal() admits
only NONE/XZ/LZ4/ZSTD. It is a trap for whoever wires up the other two, so
let's close it now.
112cfb1814 tried to improve handling of
paths when root is specified. It said:
> To avoid answering what a relative path means when called with a root
> prefix path_strv_canonicalize is now path_strv_canonicalize_absolute
> and only considers absolute paths. Fortunately all users of already call
> path_strv_canonicalize with a list of absolute paths.
Of course, other users were added later and for example now
SYSTEMD_HOSTNAME_WORDLIST_PATH=build/hostname-wordlist/ ... doesn't
work as expected. If root is not specified, there is really no reason
to ignore relative paths. They work relative to CWD per the usual convention.
It's up to the user to not use relative paths in a context where they
should not be used. When root is specified, continue to ignore the relative
paths, since there is no clear interpreation what they mean, but emit
a debug message.
For a Type=dbus service we sometimes want to keep treating it as running
while its bus name has briefly gone away (for example, due to a broker
restart).
SERVICE_RUNNING_REVALIDATING is active and keeps its main process like
SERVICE_RUNNING, but it's deliberately not part of the runtime timer
set, so RuntimeMaxSec= is paused while we sit in it.
While keyring_describe() allocates a 64 byte buffer it still passes the
value of c as the buffer length. It's -1 on the first iteration. So
keyctl_describe_key() copies the whole description whenever buflen >=
its length. So any key whose description exceeding 64 bytes overflows.
Pass the actual buffer size.
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
[zjs: adjust comment]
We copy linux/magic.h from the kernel source tree and use it when
building systemd. Let's also make the kernel-filesystem-magics test
use the copy in our tree rather than the one installed on the system.
To achieve that now the 'check' command takes both 'cpp' and
'system_include_args' from meson.
The man page udevadm(8) have explicitly documented that the option -V is
a short form of --version. Let's not hide the short form in the help
message.
This introduces OPTION_COMMON_VERSION_WITH_V then use it in udevadm,
udevd, and run0.
Alos, this moves print_version() to build.c, and renames it to
version_only(). Now the function is also used in udevd amd test-libudev.
With this change, the -V option is hidden only in dmi_memory_id and
scsi_id. So, this removes the OPTION_COMMON_VERSION_WITH_HIDDEN_V macro
and explicitly list the hidden option in their code.
Let's treat unset SYSTEMD_INVOKED_AS and SYSTEMD_INVOKED_AS='' the same.
I encountered this when testing various --help outputs. It is often
more convenient to override a variable with VAR= instead of removing it,
in particular on the commandline. But we'd then print help with the
program name missing. We look at the variable in two places, so adjust
both as in $subject.
Previously, even when the compressed data was truncated, or had
trailing garbage, the decompressor might not detect it.
To fix the issue, update the check if stream is finished.
With this change, now decompressor_push() must be called with
size == 0 to finalize the stream.
--introspect-cli follows the same deferred pattern as --help: parse_argv()
only records the action, and it is executed in the same place where
ACTION_HELP and ACTION_VERSION are handled.
The help is looked up by explicit name: in the multicall configuration
the systemd and systemd-executor commands share one binary, so the
"first COMMAND" default would depend on link order.
The "This program takes no positional arguments." line is dropped from
the help output — the default "[OPTION…]" synopsis already says so.
(And also, this is not quite true. It does take positional args when
called as init.)
In review it was raised that the file is often synthesized and that this
synthetization might not include all the fields, let's allow the more
recently added fields to be missing to avoid a regression in
compatibility.
Extend the io.systemd.Basic metric families with host-level runtime
metrics: cumulative per-state CPU seconds, memory in use (MemTotal minus
MemAvailable), swap in use, the kernel's 10s pressure stall averages
(cpu/memory/io x some/full). Partitions and stacked devices are excluded
from the disk counters, since their traffic is already accounted for in
the parent device.
Also extend the TEST-74 report subtest with checks for the new metrics.
Network metrics are not added, because the service runs with
PrivateNetwork=yes, and it doesn't make sense to report stats for 'lo'.
After some back-and-forth, I settled on "CPUUsage". We use this scheme of
capitalization in other metric family names: "IPv4Public", "IPv6Public",
IPv4AddressState", "IPv6AddressState", "OSRelease" "SMBIOS" "BootID",
"CPUsOnline", "MachineID", "MachineInfo", "OSRelease",
"SMBIOS.SystemManufacturer", "TPM2.Manufacturer", "TPM2.VendorString",
"IOReadBytes", "IOReadOperations", "CGroup". The exception is
"io.systemd.cgroup.CpuUsage". I think we should treat that one as a
mistake and consider changing it to conform.
This drops a good chunk of 21ab2fe6ec
again, which I think is plainly wrong or at the very least too
overengineered.
1. it's wrong to assume that overlayfs layers are mounted elsewhere in
the fs. it's quite likely they aren't. and overlayfs correctly passes
through fsync() to the top layer, hence we really should keep syncing
it.
2. network file systems deserve syncing. This code here runs in the
initrd, where a very restrictive set of file systems is mounted and
which runs for a very short time only, i.e. the circumstances haven't
really changed. Hence I am very sure it's better to sync file systems
here than not to, we do this for safety reasons after all.
Reliability always trumps speed here, we should not give up
correctness over presumed speed improvements.
3. There's zero reason to believe FUSE doesn't need syncing. We live in
a world where increasingly complex kernel file systems are moved to
userspace, and hence FUSE can be used to implement *anything*. Hence
we should sync it. Yes, FUSE means some backing process needs to run,
but that's a job for the initrd to get right: make sure the process
runs properly over the switch root if the mount is left in tact over
the switch root too.
Hence, dumb this all down and drop the weird fine tuning here.
Yu Watanabe
Birk Skyum
Armaan Sandhu
Lennart Poettering
Daan De Meyer
Christian Glombek
Zbigniew Jędrzejewski-Szmek
Chris Down
Christian Brauner