If the passed in image path didn't end with .raw, we'd return an empty
string + suffix instead of the intended image + suffix path.
---
Also, fix two more nits that came up repeatedly in my searches.
Every dlopen_xxx() helper now takes an int log_level argument. It is
passed through to dlopen_many_sym_or_warn() (which in turn propagates it
to dlopen_verbose() for the library-not-installed case), and is used by
the fallback stub when support for the library is not compiled in to
emit a "<lib> support is not compiled in." message at the caller's level.
Callers pass LOG_DEBUG when gracefully degrading, or a higher level when
the failure should surface, and no longer need to log redundantly at the
call site.
As part of this, dlopen_bpf_full() (which already took a log_level) is
merged into dlopen_bpf() rather than keeping both.
The static inline fallbacks used to live in the headers, which required
pulling log.h in from every header that declared a dlopen_xxx(). Move
them into the .c files instead: the declaration is always outside the
#if HAVE_XXX block, the impl sits outside the outer #if HAVE_XXX wrap
with its own internal #if HAVE_XXX/#else/#endif, and apparmor-util.c,
idn-util.c, libmount-util.c and pam-util.c are now always compiled so
they can host their stubs.
I promised in https://github.com/systemd/systemd/pull/41426 that its the
final update for coccinelle pointer deref checks. However it turned out
there is this coccinelle/parsing_hacks.h that I wasn't aware of. The
file missed some important things like _cleanup_(x) that prevented
coccinelle to check a bunch of functions.
This PR adds some missing defines to the parsing_hacks.h and fixes the
missing asserts(). I apologize that its a bit long (and frankly boring)
and that I missed this earlier.
The last commit contains one small behavior change (ret in
sd_varlink_idl_parse() is now really optional) but the big one is very
mechanical.
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.
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.
The check compared bLength against (size - sizeof(descriptor)), which
is an absolute limit unrelated to the current buffer position. Since
bLength is uint8_t (max 255), this can never exceed size - 9 for any
realistic input, making the check dead code.
Use (size - pos) instead so the check actually catches descriptors
that extend past the end of the read data.
Fixes: https://github.com/systemd/systemd/issues/41570
append_vendor_model() uses memcpy() to write VENDOR_LENGTH +
MODEL_LENGTH bytes without null-terminating. While the caller
zeroes the buffer beforehand, Coverity cannot trace this. Add
explicit null termination so the subsequent strlen() is provably
safe.
CID#1469706
Follow-up for 86fd0337c6
Per UEFI specification §13.3.2, El Torito partition discovery applies to
any block device, not just optical media. Rename
disk_get_part_uuid_cdrom() to disk_get_part_uuid_eltorito() and update
all log messages and comments to say "El Torito" instead of "CDROM" to
reflect this.
Previously, probe_gpt_sector_size_mismatch() would bail out early when
the GPT sector size matched the device sector size. However, some
devices (e.g. certain CD-ROM drives) do not support kernel partition
scanning even when sector sizes match. In that case, the kernel still
cannot parse the partition table, and we need to set up a loop device to
expose the partitions — just as we do for the sector size mismatch case.
Check blockdev_partscan_enabled() when sector sizes match, and only skip
the boot partition check if partition scanning is actually supported.
Also rename the function, udev property, and log messages to reflect the
broader scope:
- probe_gpt_sector_size_mismatch() -> probe_gpt_boot_disk_needs_loop()
- ID_PART_GPT_AUTO_ROOT_DISK_SECTOR_SIZE_MISMATCH -> ID_PART_GPT_AUTO_ROOT_DISK_NEEDS_LOOP
When the GPT partition table uses a different sector size than the
device's native block size (e.g. 512-byte GPT on a 2048-byte CD-ROM
booted via El Torito), the kernel cannot parse the partition table.
Probe the GPT sector size upfront and configure blkid with the correct
value so it always finds the partition table. If a sector size mismatch
is detected, trigger a loop device to re-expose the device with the
correct sector size and skip root partition discovery on the original
device — it will happen on the loop device instead.
Co-developed-by: Claude Opus 4.6 <noreply@anthropic.com>
strncpy does not null-terminate the destination buffer if the source
string is longer than the count parameter. Since wwn and
wwn_vendor_extension are char[17] and we copy up to 16 bytes, there's
a risk of missing null termination. Use strscpy which always
null-terminates.
CID#1469706
Follow-up for 4e9fdfccbd
The fix in 8f1751a111 made me wonder if we could automatically detect
when pointers are accessed but when this might not be safe. Systemd
is already using a lot of `assert(dst)` and this change now forces
us to use them.
So this commit (ab)uses coccinelle to flag any pointer parameter
dereference not preceded by assert(param), ASSERT_PTR(param), or an
explicit NULL check. It adds integration into meson as a new "coccinelle"
test suite (just like clang-tidy) and is run in CI. The check is not
perfect but seems a reasonable heuristic.
For this RFC commit it is scoped to a subset, it excludes 25 dirs right
now and includes around 100. About 300 warnings left. Busywork that I am
happy to do if there is agreement that it is worth it.
With this in place we would have caught the bug from 8f1751a111 in CI:
```
FAIL: check-pointer-deref.cocci found issues in systemd/src/boot:
diff -u -p systemd/src/boot/measure.c /tmp/nothing/measure.c
--- systemd/src/boot/measure.c
+++ /tmp/nothing/measure.c
@@ -312,7 +312,6 @@ EFI_STATUS tpm_log_tagged_event(
if (err != EFI_SUCCESS)
return err;
- *ret_measured = true;
return EFI_SUCCESS;
}
```
This also adds a new POINTER_MAY_BE_NULL() for the cases when the
called function will do the NULL check (like `iovec_is_set()`).
A 10G Marvell AQC113 included in an ASRock TRX50WS motherboard NIC claims to
support tx/rx queues as large as 8184.
After boot 'ethtool -g eth0' outputs:
Ring parameters for eth0:
RX: 8184
RX Mini: n/a
RX Jumbo: n/a
TX: 8184
TX push buff len: n/a
HDS thresh: n/a
RX: 2048
RX Mini: n/a
RX Jumbo: n/a
TX: 4096
RX Buf Len: n/a
CQE Size: n/a
TX Push: off
RX Push: off
TX push buff len: n/a
TCP data split: n/a
HDS thresh: n/a
'ethtool --set-ring eth0 rx 8184 tx 8184 && ethtool -g eth0' yields:
Ring parameters for eth0:
RX: 8184
RX Mini: n/a
RX Jumbo: n/a
TX: 8184
TX push buff len: n/a
HDS thresh: n/a
RX: 8184
RX Mini: n/a
RX Jumbo: n/a
TX: 8184
RX Buf Len: n/a
CQE Size: n/a
TX Push: off
RX Push: off
TX push buff len: n/a
TCP data split: n/a
HDS thresh: n/a
I can measure a throughput difference between using using buffer sizes
4096 and 8184 on my hardware, so it really seems that this is doing
something beyond buggy firmware.
Original PR https://github.com/systemd/systemd/pull/17635 didn't give any
explanation for the limit of 4096, but that's probably what was supported by
the kernel drivers at the time.
A web search shows that CISCO VIC 15000 supports 16k, so allow up to that.
[zjs: edited the message]
We often have a pattern where the same verb function is used for
multiple actions. This leads to an antipattern where we figure out what
action needs to be taken from argv[0] multiple times: often once in
parse_argv() to figure out what options are allowed, then once again
implicitly in dispatch_verb(), and then again in the action verb itself.
Let's allow passing a parameter into the verb to simplify this.
This matches a pattern we have in conf-parser.h, where we have both
void *userdata (more global) and void *data (per-config item). Here,
I opted for uintptr_t userdata. It seems that most of the time we'll
want to just pass an enum value. This works OK with no casts. I also
tried a void* and union. In both cases, much more boilerplate is needed:
either a cast or a macro to help avoid compiler warnings. uintptr_t
seems generic enough to cover foreseeable usecases with no fuss.
This is a noop refactoring. See next commit for an example.
The previous version in the PR changed variable and sanitized it
in place. The second version switched to skip if CCs are in the
string instead, but didn't move back to the original variable.
Because it's an existing variable, no CI caught it.
Follow-up for 16325b35fa
The removed code in device_tag_index() in fact does nothing,
as sd_device.all_tags is never cleared. Moreover, not only the code
is meaningless, but it is theoretically/logically wrong, as the symlinks
in /run/udev/tags/ should be 'sticky', hence we should even not try to
remove them.
The current tag concept has been introduced by
e77b146f82 (v247) to make symlinks in
/run/udev/tags/ are 'sticky'.
However, when TAG= (rather than TAG+=) is specified, then the tags
assigned in the previous events were also cleared.
This fixes the issue and now symlinks in /run/udev/tags/ are really
'sticky'.
Fortunately, TAG= is mostly unused. So, the issue should not affect
and the fix should not change anything on almost all systems.
XDG_SESSION_EXTRA_DEVICE_ACCESS will now take a colon-separated list of
identifiers. For every identifier $ID, the session is granted access to all
devices tagged as "xaccess-$ID" in udev.
Fixes: #40634
This reverts (in sprit) commit f5cdf9515a,
"udev-rules: ignore non-system user/group in OWNER=/GROUP=".
The original change was done to clean up a situation where we added a
new group, but the group could already have been used for some other
purposes, and now the some unexpected entity would own the device.
Unfortunately, this check doesn't really address the issue, since the
existing account might as well be a system account, which might be
equally bad. In addition, this change is a big compatiblity break,
causing existing rules to stop working. Since quite a lot of systems
have local configuration to assign devices to users for various
purposes, this is very noticable to users. In a way, the original change
to add a new group was the compat break, and follow-up patch to cahnge
the rule parsing evolved a small compat break into a much bigger one.
There is merit to the change though, since device nodes shouldn't be
owned by users and groups and different mechanisms should be used
instead. To avoid breaking users systems, and since the original goal
cannot be achieved by this patch, let's downgrade this to a warning
to guide users towards different solutions.
When systemd is compiled with group-render-mode=0660, only the active
seat gets access to the render devices through uaccess. Remote desktop
sessions like gnome-remote-desktop would be left with no hardware
rendering, because those sessions are not associated with a seat.
We solve the issue by granting uaccess to specifically tagged devices on
session start, if the session is marked with
XDG_SESSION_EXTRA_DEVICE_ACCESS.
udev-builtin-uaccess is refactored to grant multiple users access to a
device, taking into account the device's seat and all the active
EXTRA_DEVICE_ACCESS sessions.
This partially reverts 36c557f7d4, which
introduced notify_remove_fd() that logs in LOG_DEBUG. However,
notify_remove_fd_warn() is still called other library functions, e.g.
notify_push_fd(), and produces warning message about the failure in
removing fd from fdstore on shutdown.
During shutdown process, we get the following logs:
```
systemd-udevd[370]: Failed to send notify message to '/run/systemd/notify': Connection refused
systemd-udevd[370]: Failed to remove file descriptor "config-serialization" from the store, ignoring: Connection refused
systemd-udevd[370]: Failed to send notify message to '/run/systemd/notify': Connection refused
systemd-udevd[370]: Failed to push serialization fd to service manager: Connection refused
```
Here, the 1st, 3rd, and 4th messages are in LOG_DEBUG, but the 2nd one
was in LOG_WARNING before this commit, and this makes it also in LOG_DEBUG.
Follow-up for 472404aca5.
Grant access to devices tagged with "xaccess" on session start, if the session
was created with XDG_SESSION_EXTRA_DEVICE_ACCESS=1.
udev-builtin-uaccess is refactored to grant multiple users access to a device,
taking into account the device's seat and all the active EXTRA_DEVICE_ACCESS
sessions.
Suppose the following dependent events are queued:
A(1000) -> C(1002)
\
--> E(1004)
/
B(1001) -> D(1003)
Here, each number is the seqnum of the event, and A -> B means that
event B depends on event A, that is, event B cannot be started until
event A is processed.
In this case, to know if event E can be started, we only need to check
if event C and D are processed, and not necessary to check the state of
event A or B.
However, the commit e1ae931064 introduced
full dependency list (in the above example, all A, B, C, D are listed as
the blocker for E), but it is overkill and most information is not necessary.
Also, before the commit e1ae931064, we
found blocker from the beginning of the queued events, but that's also
not necessary, as even A is processed, still C may be queued, and we
anyway need to check if C is processed or not.
This makes each event only stores the last blocker event for the event,
and finds the blocker from the end of the queue. With this change, the
memory cost can be reduced from O(n^2) to O(n). Also, as previously we
used Set for managing blockers, but now we only increment/decrement
the reference counter of events, so the speed should be also improved.
Fixes#39583 and #39817.
When udevd received a uevent or inotify event, we will queue or trigger
synthesized events. To minimize the race between processing these events
by udevd and user invocation of 'udevadm settle', let's create /run/udev/queue
file earlier.
On some errors, no event may be queued, but in that case, the file will
be removed by the post event source if nothing queued. See on_post().
Hopefully mitigate the issue #40499.
Otherwise, /run/udev/queue file may be removed before udevd receives
the triggered uevents. This avoids the following race:
1. uevent A is being processed,
2. received an inotify event and trigger a synthesized uevent,
3. uevent A has been processed,
4. the uevent queue becomes empty, and /run/udev/queue is removed,
5. received and queued the synthesized uevent, and /run/udev/queue is recreated.
With this patch, /run/udev/queue is not removed in step 4. Hence,
'udevadm settle' will wait for the synthesized event being processed.
This is similar to waiting for child processes that reread partitions
(thus trigger synthetic events) being terminated, but for synthetic
events triggered by the manager.
Zbigniew Jędrzejewski-Szmek
Daan De Meyer
Frantisek Sumsal
Luca Boccassi
Michael Vogt
Milan Kyselica
Nick Rosbrook
Walter McKelvie
Yu Watanabe
Lennart Poettering
Alessandro Astone