gcc does not inline sipround() at -O2: its size estimate lands just over
the default max-inline-insns-auto budget, so nm on libsystemd-shared.so
shows a local sipround symbol and each of the ten call sites pays a
call/ret plus a reload and store of the state through the pointer, around
six rounds of ~14 ALU ops. clang inlines it on its own, and gcc does too
at -O3 or with --param max-inline-insns-auto=30, so this is not a missed
optimization to report upstream but the inliner's size heuristic doing
what it is designed to do. The documented way to overrule it for one
function is always_inline.
The SipHash reference implementation this file is derived from inlines
the round manually, by writing it as a SIPROUND macro.
AMD Ryzen AI 7 350, gcc 16.2.1, -O2 -fstack-protector-strong
-fno-strict-aliasing, median of five runs:
input before after speedup
8 B 61.3 ns 9.3 ns 6.63x
16 B 54.6 ns 10.7 ns 5.11x
24 B 50.0 ns 12.6 ns 3.98x
40 B 49.4 ns 16.4 ns 3.01x
64 B 53.2 ns 22.1 ns 2.41x
128 B 67.8 ns 36.9 ns 1.84x
512 B 157.7 ns 128.1 ns 1.23x
4096 B 992.5 ns 964.5 ns 1.03x
The win is concentrated on short inputs because the bulk loop in
siphash24_compress() was the one place gcc did inline the round; the
prologue, the tail and all six rounds of siphash24_finalize() were not.
Short inputs are also what we overwhelmingly hash: hashmap and set keys,
unit names, paths, journal field names and data objects, bus match
strings. clang is unaffected.
Resulting hash values are unchanged; verified identical over every input
length from 0 to 128 bytes, and test-siphash24, test-hash-funcs,
test-hashmap and test-set pass.
journal_entry_to_json() builds a fresh hashmap for every entry and then fills
it with that entry's fields, of which there are typically around twenty. Since
the hashmap starts out empty it has to grow several times per entry, and every
growth rehashes everything inserted so far. journalctl -o json spent about a
quarter of its time in siphash, a third of that under resize_buckets().
Reserve room up front. On a 730M journal spread over 100 files,
journalctl -o json -n 200000 goes from 5.71s to 4.64s.
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.
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.
manager_recv() sizes its buffer with next_datagram_size_fd(), which peeks at
the datagram with MSG_PEEK|MSG_TRUNC. That peek makes the kernel verify the UDP
checksum - which is the whole point of using it over FIONREAD - and hence also
drop the datagram when the checksum is bad. The datagram that woke the event
source can thus be gone by the time we peek at it, and the peek returns EAGAIN.
manager_recv() returned that error, while the recvmsg_safe() below, which can
fail the same way, already treats it as "no packet". The callers on the mDNS,
LLMNR and stub listening sockets pass a negative return on to the event loop,
which disables the event source, so a single datagram with a bad checksum -
something anyone on the link can send - stops resolved from receiving on that
socket for good. on_dns_packet() completes the transaction with -EAGAIN instead
of retrying it.
So return 0 on a transient error here too, like the other users of the helper
on an event source do. Disconnect errors are left as they are: they show up
here from ICMP errors on the connected UDP socket, and on_dns_packet() handles
them.
mdns_maintenance_query_complete() takes a reference on
query->dnsservice_request and never drops it, so every successful mDNS
maintenance query leaks one reference to the discovered service.
The leak alone would be benign, but the service owns the one-shot event
source that drives its maintenance schedule, and it only borrows a plain
pointer to its service browser. Once the extra reference is outstanding,
dns_service_browser_free() -> dns_remove_service() only drops the list's
reference, so dns_service_free() never runs, the maintenance timer is
never disabled, and the service outlives the browser it points at. The
next time the timer fires, mdns_maintenance_query() dereferences the
freed DnsServiceBrowser to reach the manager's event loop and to issue
the query.
Reaching it only takes a browse subscription whose record lives long
enough for one maintenance query to be answered, followed by the
subscription going away.
Make the reference a _cleanup_ one, the way the sibling
mdns_browse_service_query_complete() below already handles the reference
it takes on the service browser.
systemd-importd runs qcow2_detect()/qcow2_convert() over images it has just
downloaded (see pull-raw.c), so the header, the L1/L2 tables and every offset
and cluster descriptor in them are attacker controlled. There is a manual test
for the happy path, but nothing that feeds it a malformed image.
The corpus is two images, a plain data cluster and a zlib-compressed one (i.e.
decompress_zlib_raw()), at the 512 byte minimum cluster size and trimmed to the
last byte the converter reads, so 2K each. A zero cluster and a QCOW3 header are
a flipped descriptor bit and a bumped version field away from these, so they are
left to the fuzzer rather than carried in the tree.
fuzz-compress only ever feeds the decompressors their own output: it compresses
the fuzzer input and then decompresses the result, so the stream the decoders
see is well formed by construction. The blobs they get in reality come out of
journal files, systemd-journal-remote streams, coredumps and downloaded images,
i.e. from wherever the data was written, which is not necessarily somewhere we
trust.
So hand them the fuzzer input directly instead, across all four entrypoints:
decompress_blob(), decompress_startswith(), decompress_stream(), and the
decompressor_detect()/decompressor_push() pair that systemd-importd feeds from
the network. The latter is pushed in chunks, so that the decoders see the same
fragmentation a real download produces. The decompressed size is capped, as an
unbounded decode of a hostile blob measures the allocator rather than the
decoder.
The corpus carries the two truncated stream headers that used to send
decompress_startswith() allocating until OOM, fixed in the previous commit.
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.
A newline embedded in an input entry only reset the width counter and the
leading-whitespace state, but the text collected before it was never flushed.
If nothing but whitespace followed the newline, the end-of-line path then saw
that we were still in the leading whitespace and emitted an empty line,
dropping everything before the newline: "foo\n" turned into "".
Newlines also didn't start a new output entry, so returned entries could
contain newlines and be wider than the requested width, even though callers
expect one output entry per output line.
Flush the collected line at the newline instead, reusing the same logic as at
the end of the string, and treat a trailing newline as terminating the last
line rather than adding an empty one.
An argument of the form "--=VALUE" leaves an empty option name, which
startswith() treats as a prefix of every defined option. Depending on how many
long options a namespace defines that yields either an ambiguity error listing
every option, or, with exactly one option defined, a unique partial match that
silently parses "--=VALUE" as that option.
The backwards scan started at offset length-3, so the last position at which a
sequence can begin, length-2, was never examined. CSI sequences are at least
three bytes long and were thus unaffected, but two byte Fe sequences (ESC
followed by 0x40…0x5F) terminating the examined slice were missed.
ellipsize_mem() calls this to figure out whether a sequence ends exactly at the
current position, so that it can be skipped over, which is precisely the case
that was broken: such a sequence was instead counted as two visible cells and
copied through as text, and the string was ellipsized more aggressively than
requested. For example ellipsize("🐱🐱\x1bM🐱🐱\x1bM", 5, 0) returned a three
cell wide string rather than the five cells asked for.
The message, icon and id fields are written verbatim into single-line
assignments of the [Ask] section of the agent request file, so a newline in
them lets the caller append arbitrary further assignments. Agents let a later
assignment override an earlier one, so an injected Socket= line redirects the
password to a path of the injector's choosing. Validate the fields and refuse
the request instead.
The scan pointer was advanced with utf8_next_char(), which blindly skips
utf8_skip_data[lead byte] bytes, so a line ending in a truncated multibyte
sequence like "foo\xF0" stepped over the NUL and the loop read past the end
of the string. Decode each character with utf8_encoded_to_unichar() and
propagate the error instead of measuring broken characters.
Fixes#43052.
Move the --reboot/--component= rejection into parse_argv() alongside the
other cross-option checks, and tighten TEST-72 to assert the specific
guard message rather than merely a non-zero exit.
The `pending` and `reboot` verbs, as well as the `--reboot` switch, compare
the newest installed version against the booted OS version (IMAGE_VERSION= from
os-release). When a component is selected via --component=, this compares the
component's version against the unrelated host OS version, which by design live
in separate version spaces. The result is arbitrary reboot decisions: depending
on the relative version strings sysupdate would either always or never reboot.
Refuse the combination with a clear error instead of silently performing a
bogus comparison. Correctly tracking a per-component booted version is left as a
future feature.
Fixes: https://github.com/systemd/systemd/issues/42330
Armaan Sandhu