We already have checks in place during compression that limit the data
we compress, so they shouldn't decompress to anything larger than
DATA_SIZE_MAX unless they've been tampered with. Let's make this
explicit and limit all our decompress_blob() calls in journal-handling
code to that limit.
One possible scenario this should prevent is when one tries to open and
verify a journal file that contains a compression bomb in its payload:
```
$ ls -lh test.journal
-rw-rw-r--+ 1 fsumsal fsumsal 1.2M Apr 12 15:07 test.journal
$ systemd-run --user --wait --pipe -- build-local/journalctl --verify --file=$PWD/test.journal
Running as unit: run-p682422-i4875779.service
000110: Invalid hash (00000000 vs. 11e4948d73bdafdd)
000110: Invalid object contents: Bad message
File corruption detected at /home/fsumsal/repos/@systemd/systemd/test.journal:272 (of 1249896 bytes, 0%).
FAIL: /home/fsumsal/repos/@systemd/systemd/test.journal (Bad message)
Finished with result: exit-code
Main processes terminated with: code=exited, status=1/FAILURE
Service runtime: 48.051s
CPU time consumed: 47.941s
Memory peak: 8G (swap: 0B)
```
Same could be, in theory, possible with just `journalctl --file=`, but
the reproducer would be a bit more complicated (haven't tried it, yet).
Lastly, the change in journal-remote is mostly hardening, as the maximum
input size to decompress_blob() there is mandated by MHD's connection
memory limit (set to JOURNAL_SERVER_MEMORY_MAX which is 128 KiB at the
time of writing), so the possible output size there is already quite
limited (e.g. ~800 - 900 MiB for xz-compressed data).
Convert string fields to varlink enums in io.systemd.Unit
Following
https://github.com/systemd/systemd/pull/39391#discussion_r2489599449,
convert all configuration setting fields in the io.systemd.Unit varlink
interface from bare SD_VARLINK_STRING to proper enum types, adding type
safety to the IDL.
This converts ~30 fields across ExecContext, CGroupContext, and
UnitContext, adding 25 new varlink enum types.
Weak compatibility breakage (per
https://github.com/systemd/systemd/pull/40972#issuecomment-4222294318):
Varlink enum identifiers cannot contain - or +, so affected values are
underscorified on the wire. For example, "tty-force" becomes tty_force,
"kmsg+console" becomes kmsg_console.
The full list of affected values:
```
- ExecInputType: tty-force, tty-fail
- ExecOutputType: kmsg+console, journal+console
- ProtectHome: read-only
- CGroupController: bpf-firewall, bpf-devices, bpf-foreign, bpf-socket-bind, bpf-restrict-network-interfaces, bpf-bind-network-interface
- CollectMode: inactive-or-failed
- EmergencyAction: exit-force, reboot-force, reboot-immediate, poweroff-force, poweroff-immediate, soft-reboot, soft-reboot-force, kexec-force, halt-force, halt-immediate
- JobMode: replace-irreversibly, ignore-dependencies, ignore-requirements, restart-dependencies
```
We already have checks in place during compression that limit the data
we compress, so they shouldn't decompress to anything larger than
DATA_SIZE_MAX unless they've been tampered with. Let's make this
explicit and limit all our decompress_blob() calls in journal-handling
code to that limit.
One possible scenario this should prevent is when one tries to open and
verify a journal file that contains a compression bomb in its payload:
$ ls -lh test.journal
-rw-rw-r--+ 1 fsumsal fsumsal 1.2M Apr 12 15:07 test.journal
$ systemd-run --user --wait --pipe -- build-local/journalctl --verify --file=$PWD/test.journal
Running as unit: run-p682422-i4875779.service
000110: Invalid hash (00000000 vs. 11e4948d73bdafdd)
000110: Invalid object contents: Bad message
File corruption detected at /home/fsumsal/repos/@systemd/systemd/test.journal:272 (of 1249896 bytes, 0%).
FAIL: /home/fsumsal/repos/@systemd/systemd/test.journal (Bad message)
Finished with result: exit-code
Main processes terminated with: code=exited, status=1/FAILURE
Service runtime: 48.051s
CPU time consumed: 47.941s
Memory peak: 8G (swap: 0B)
Same could be, in theory, possible with just `journalctl --file=`, but
the reproducer would be a bit more complicated (haven't tried it, yet).
Lastly, the change in journal-remote is mostly hardening, as the maximum
input size to decompress_blob() there is mandated by MHD's connection
memory limit (set to JOURNAL_SERVER_MEMORY_MAX which is 128 KiB at the
time of writing), so the possible output size there is already quite
limited (e.g. ~800 - 900 MiB for xz-compressed data).
We have a test failure where the testsuite is calling
sd_varlink_idl_parse() with *ret being NULL. This is now an
assert error. So we could either fix the test or fix the code
Given that it seems genuinely useful to run sd_varlink_idl_parse()
without *ret to e.g. just check if the idl is valid I opted to
fix the code.
The updated parsing_hacks.h file uncovered a bunch of extra
things that the check-pointer-deref coccinelle script flags.
This commit fixes them to make the tree check-pointer-deref clean.
1024 connections per UID is unnecessarily generous, so let's scale this
down a bit. D-Bus defaults to 256 connections per UID, but let's be even
more conservative and go with 128.
When JSON_STREAM_INPUT_SENSITIVE is set before the first read,
input_buffer is NULL, input_buffer_size is 0, and input_buffer_index
is 0. The old condition '!INPUT_SENSITIVE && index == 0' would route
this case into the else branch which calls memcpy() with a NULL source
pointer, which is undefined behavior even when the length is zero, and
is caught by UBSan.
Fix by checking input_buffer_index == 0 first, then allowing the
GREEDY_REALLOC fast path also when input_buffer_size == 0, since
there is no sensitive data to protect from realloc() copying in that
case. The else branch is now only entered when there is actual data
to copy (input_buffer_size > 0), guaranteeing input_buffer is
non-NULL.
Follow-up for 6b1a59d594
Introduces JsonStream, a generic transport layer for JSON-line message
exchange over a pair of file descriptors. It owns the input/output
buffers, SCM_RIGHTS fd passing, the deferred output queue, the
read/write/parse step functions, sd-event integration (input/output/time
event sources), the idle timeout machinery, and peer credential caching,
but knows nothing about the specific JSON protocol on top — the consumer
drives its state machine via phase/dispatch callbacks supplied at
construction.
sd-varlink is reworked to delegate the entire transport layer to a
JsonStream owned by sd_varlink. The varlink struct drops every
transport-related field (input/output buffers and fds, output queue,
fd-passing state, ucred/pidfd cache, prefer_read/write fallback, idle
timeout, description, event sources) — all of that lives in JsonStream
now. What remains in sd_varlink is the varlink-protocol state machine
(state, n_pending, current/previous/sentinel, server linkage, peer
credentials accounting, exec_pidref, the varlink-specific quit and defer
sources) and a thin wrapper layer over the JsonStream API. The
should_disconnect / get_timeout / get_events / wait helpers all live in
JsonStream now and are driven by a JsonStreamPhase the consumer reports
via its phase callback.
Define proper varlink enum types for unit configuration settings that
are part of the user-facing API (values users/clients can select).
This replaces SD_VARLINK_STRING with SD_VARLINK_DEFINE_FIELD_BY_TYPE
for these fields, giving them strong type semantics in the IDL.
Enum types added for ExecContext (ExecInputType, ExecOutputType,
ExecUtmpMode, ExecPreserveMode, ExecKeyringMode, MemoryTHP,
ProtectProc, ProcSubset, ProtectSystem, ProtectHome, PrivateTmp,
PrivateUsers, ProtectHostname, ProtectControlGroups, PrivatePIDs,
PrivateBPF), CGroupContext (CGroupDevicePolicy, ManagedOOMMode,
ManagedOOMPreference, CGroupPressureWatch, NFTSetSource, NFProto,
BPFCGroupAttachType, CGroupController), and UnitContext (CollectMode,
EmergencyAction, JobMode).
Engine-reported runtime state fields (Type, LoadState, ActiveState,
FreezerState, SubState, UnitFileState) remain as strings since only
the engine selects those values.
As mentioned in the grandfather commit, I want to use the _many
suffix for freeing of the contents of an array, so the functions
to free the array to get the suffix _array.
newa(t, n) already allocates sizeof(t) * n bytes, so previously we'd
actually allocate sizeof(t) * sizeof(t) * n bytes, which is ~16x more
(on x86_64) that we actually needed.
This is probably an oversight from a tree-wide change in
6e9417f5b4 that replaced alloca() with
newa().
Follow-up for 6e9417f5b4.
With the old version there was a potential connection count leak if
either of the two hashmap operations in count_connection() failed. In
that case we'd return from sd_varlink_server_add_connection_pair()
_before_ attached the sd_varlink_server object to an sd_varlink object,
and since varlink_detach_server() is the only place where the connection
counter is decremented (called through sd_varlink_close() in various
error paths later _if_ the "server" object is not null, i.e. attached to
the sd_varlink object) we'd "leak" a connection every time this
happened. However, the potential of abusing this is very theoretical,
as one would need to hit OOM every time either of the hashmap operations
was executed for a while before exhausting the connection limit.
Let's just increment the connection counter after any potential error
path, so we don't have to deal with potential rollbacks.
Move the push-based streaming compression API from import-compress.c
into compress.c and delete import-compress.c/h. This consolidates all
compression code in one place and makes all compression libraries
(liblzma, liblz4, libzstd, libz, libbz2) runtime-loaded via dlopen
instead of directly linked.
Introduce opaque Compressor/Decompressor types backed by a heap-
allocated struct defined only in compress.c, keeping all third-party
library headers out of compress.h.
Rewrite the per-codec fd-to-fd stream functions as thin wrappers around
the push API via generic compress_stream()/decompress_stream() taking a
Compression type parameter. Integrate LZ4 into this framework using the
LZ4 Frame API, eliminating all LZ4 special-casing.
Extend the Compression enum with COMPRESSION_GZIP and COMPRESSION_BZIP2
and add the corresponding blob, startswith, and stream functions for
both.
Rename the ImportCompress types and functions: ImportCompressType becomes
the existing Compression enum, ImportCompress becomes Compressor (with
Decompressor typedef), and all import_compress_*/import_uncompress_*
become compressor_*/decompressor_*. Rename dlopen_lzma() to dlopen_xz()
for consistency. Make compression_to_string() return lowercase by
default.
Add INT_MAX/UINT_MAX overflow checks for LZ4, zlib, and bzip2 blob
functions where the codec API uses narrower integer types than our
uint64_t parameters.
Migrate test-compress.c and test-compress-benchmark.c to the TEST()
macro framework, new assertion macros, and codec-generic loops instead
of per-codec duplication.
Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
When there is a potential protocol upgrade we need to be careful that
we do not read beyond our json message as the custom protocol may be
anything. This was archived via a byte-by-byte read. This is of course
very inefficient. So this commit moves to use MSG_PEEK to find the
boundary of the json message instead. This makes the performance hit
a lot smaller.
Thanks to Lennart for suggesting this.
When the server side of a varlink connection supports connection
upgrades we need to go into single byte-read mode to avoid the
risk of a client that sends the json to protocol upgrade and then
immediately the custom protocol payload. This commit implements
this.
The next step is using MSG_PEEK to avoid the single-byte overhead.
This commit tweaks the API of sd_varlink_call_and_upgrade and
sd_varlink_reply_and_upgrade to return two independent fds even
if the internal {input,output}_fd are the same (e.g. a socket).
This makes the external API easier as there is no longer the risk
of double close. The sd_varlink_call_and_upgrade() is not in a
released version of systemd yet so I presume it is okay to update
it still.
This also allowed some simplifications in varlinkctl.c now that
the handling is easier.
Extract the fd-handling logic from sd_varlink_call_and_upgrade() into a
shared static helper so that it can be reused by the upcoming server-side
sd_varlink_reply_and_upgrade().
`print_list()` and `print_info()` used `RETRIEVE()` to `strndup()` the
entire
`COREDUMP` field into a heap-allocated string, only to check whether it
exists.
With `sd_journal_set_data_threshold(j, 0)` in `print_info()`, this
copies the
full coredump binary (potentially hundreds of MB) to heap just to print
"Storage: journal".
This PR:
1. Makes `sd_journal_get_data()` output parameters optional
(`NULL`-safe), so
callers can do pure existence checks without receiving the data.
2. Short-circuits `maybe_decompress_payload()` after
`decompress_startswith()`
succeeds when neither output pointer is requested, skipping full blob
decompression for compressed journal entries.
3. Switches coredumpctl to pass `NULL, NULL` for the existence checks
instead
of heap-copying via `RETRIEVE()`.
When both ret_data and ret_size are NULL after decompress_startswith()
has confirmed the field matches, skip the decompress_blob() call.
This avoids decompressing potentially large payloads (e.g. inline
coredumps) just to discard the result.
Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
Allow callers to pass NULL for ret_data and/or ret_size when they only
need to check whether a field exists. Initialize provided output
pointers to safe defaults and update the manual page accordingly.
Propagate the NULL-ness through to journal_file_data_payload() so that
downstream helpers can optimize for the existence-check case.
Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
Coverity flags si.ssi_signo as tainted data from read(), and warns
that casting it to signed could produce a negative value. Add an
explicit range check against INT_MAX before the SIGNAL_VALID check
to prove the cast is safe.
CID#1548033
Follow-up for c8b53fcfd3
Coverity flags ALIGN(sizeof(sd_bus_message)) as potentially
returning SIZE_MAX, making the subsequent + sizeof(BusMessageHeader)
overflow. Store the ALIGN result in a local and assert it is not
SIZE_MAX.
CID#1548031
Follow-up for 4f5b28b72c
Coverity flags ALIGN() as potentially returning SIZE_MAX and the
subsequent a += label_sz + 1 as overflowing. Assert ALIGN result
is not SIZE_MAX and use INC_SAFE for the addition.
CID#1548030
Follow-up for 55354d5930
Coverity flags sizeof(BusMessageHeader) + ALIGN8(m->fields_size)
as overflowing because ALIGN_TO can return SIZE_MAX as an overflow
sentinel. Assert that the aligned value is not SIZE_MAX to prove
the addition is safe.
CID#1548023
CID#1548046
Follow-up for 2ac7c17f9d
sd_json_variant_unsigned() incorrectly calls sd_json_variant_integer()
for reference-type variants instead of recursing to itself. This silently
returns 0 for unsigned values in the range INT64_MAX+1 through
UINT64_MAX, since sd_json_variant_integer() cannot represent them.
The sibling functions sd_json_variant_integer() and
sd_json_variant_real() correctly recurse to themselves.
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
This is a followup to https://github.com/systemd/systemd/pull/41400 with
the final set of tweaks so that the new coccinelle `check-pinters-deref`
checker runs without failures.
It also includes a commit with some fixes for redundant asserts and the
final commit to make the assert() in the inner loop of qsort
POINTER_MAY_BE_NULL which will become a no-op - that is probably the
most controversial one, I hope the commit message explains the
trade-offs, I'm happy to drop it, I have no strong opinion either way.
Please see the individual commits/commit messages for more details, most
is (hopefully) relatively boring/mechanical.
This commit fixes an issue with the fd handling in
sd_varlink_call_and_upgrade() when one direction of the output
FDs is unset.
Thanks to Lennart for spotting this and suggesting the fix.
The varlink spec supports protocol upgrades and they are very
useful to e.g. transfer binary data directly via varlink. So
far sd-varlink was not supporting this.
This commit adds a new public sd_varlink_call_and_upgrade()
that sends a method call, waits for the reply, then steals
the connection fds for raw I/O. It returns separate input_fd
and output_fd to support both bidirectional sockets and pipe
pairs.
A helper is extracted and shared between sd_varlink_call_full()
and sd_varlink_call_and_upgrade(). A new `protocol_upgrade`
bool in `struct sd_varlink` ensures that on a protocol upgrade
request we only exactly read the varlink protocol bytes and
leave anything beyond that to the caller that speaks the upgraded
protocol.
Note that this is the client side of the library implementation
only for now. The server side needs work but this is already
useful as it allows to talk to varlink servers that speak protocol
upgrades (like the rust implemenations of varlink).
Use CONST_ALIGN_TO to express the compile-time overflow check for
the ALIGN(sizeof(sd_bus_message)) + sizeof(BusMessageHeader)
allocation, since ALIGN() is not constexpr.
CID#1548031
Follow-up for de1c301ed1
Coverity flags arithmetic in BUS_MESSAGE_SIZE(),
BUS_MESSAGE_BODY_BEGIN() and message_from_header() as potential
overflows. The values are validated at message creation time, but
add asserts to make the invariants explicit for static analyzers.
CID#1548023
CID#1548030
CID#1548046
Follow-up for 6629161f82
Coverity flags the subtraction from msg_controllen as a potential
underflow. The CMSG_SPACE was added when send_ucred was set, and
the subtraction only runs when send_ucred was true, so it is safe.
Add an assert to document this invariant.
CID#1548074
Follow-up for 64144440a5
Same pattern as the fix for sd_json_variant_unset_field in
9b3715d529: cache the element
count in a local variable and assert it is at least 2 before
subtracting.
CID#1548029
Follow-up for f2ff34ff2a
Coverity flags max_size*2 as a potential overflow. The value is
bounded by MAX_SIZE_UPPER (128 MiB) or JOURNAL_COMPACT_SIZE_MAX
(4 GiB), so doubling is safe within uint64_t. Add an assert to
document this.
CID#1548019
Follow-up for 8580d1f73d
Lennart Poettering
Zbigniew Jędrzejewski-Szmek
Frantisek Sumsal
Michael Vogt
Luca Boccassi
Daan De Meyer
Ivan Kruglov
noxiouz
Christian Brauner
Christian Brauner