Avoid passing a NULL message to sd_bus_message_is_signal(), to not trip
over an assertion:
[ 132.869436] H testsuite-82.sh[614]: + systemctl --no-block --check-inhibitors=yes soft-reboot
[ 132.967386] H systemd[1]: Created slice system-systemd\x2dcoredump.slice.
[ 133.018292] H systemd[1]: Starting inhibit.service...
[ 133.122610] H systemd[1]: Started systemd-coredump@0-665-0.service.
[ 133.163643] H systemd[1]: Started inhibit.service.
[ 133.206836] H testsuite-82.sh[614]: + exec sleep infinity
[ 133.236762] H systemd-logind[611]: The system will reboot now!
[ 135.891607] H systemd-coredump[667]: [🡕] Process 663 (busctl) of user 0 dumped core.
Stack trace of thread 663:
#0 0x00007f2ec45e6acf raise (libc.so.6 + 0x4eacf)
#1 0x00007f2ec45b9ea5 abort (libc.so.6 + 0x21ea5)
#2 0x00007f2ec4b5c9a6 log_assert_failed (libsystemd-shared-255.so + 0x1ff9a6)
#3 0x00007f2ec4b5dca5 log_assert_failed_return (libsystemd-shared-255.so + 0x200ca5)
#4 0x00007f2ec4bb3df6 sd_bus_message_is_signal (libsystemd-shared-255.so + 0x256df6)
#5 0x000000000040e478 monitor (busctl + 0xe478)
#6 0x000000000040e82f verb_monitor (busctl + 0xe82f)
#7 0x00007f2ec4b202cb dispatch_verb (libsystemd-shared-255.so + 0x1c32cb)
#8 0x00000000004074fa busctl_main (busctl + 0x74fa)
#9 0x0000000000407525 run (busctl + 0x7525)
#10 0x000000000040ff67 main (busctl + 0xff67)
#11 0x00007f2ec45d2d85 __libc_start_main (libc.so.6 + 0x3ad85)
#12 0x00000000004044be _start (busctl + 0x44be)
ELF object binary architecture: AMD x86-64
[ 136.141152] H dbus-daemon[634]: [system] Monitoring connection :1.2 closed.
[ 136.152233] H systemd[1]: busctl.service: Main process exited, code=dumped, status=6/ABRT
[ 136.153996] H systemd[1]: busctl.service: Failed with result 'core-dump'.
The asertion in question:
Assertion 'm' failed at src/libsystemd/sd-bus/bus-message.c:1015, function sd_bus_message_is_signal(). Aborting.
We can get a NULL message here through sd_bus_process() ->
bus_process_internal() -> process_running(), so let's handle this case
appropriately.
This is pretty much the same stuff as `resolvectl monitor` does, and
allows us to run `busctl monitor` in a Type=notify unit which ensures
that `busctl` is really listening for messages once the unit is marked
as started.
This allows using it with CLEANUP_ARRAY(). For the 2 call sites
where we don't need to free the array, we do a regular for loop
calling json_variant_unref() instead.
When closing the FILE handle attached to a memstream, it may attempt to
do a realloc() that may fail during OOM situations, in which case we are
left with the buffer pointer pointing to NULL and buffer size > 0. For
example:
```
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
void *realloc(void *ptr, size_t size) {
return NULL;
}
int main(int argc, char *argv[])
{
FILE *f;
char *buf;
size_t sz = 0;
f = open_memstream(&buf, &sz);
if (!f)
return -ENOMEM;
fputs("Hello", f);
fflush(f);
printf("buf: 0x%lx, sz: %lu, errno: %d\n",
(unsigned long) buf, sz, errno);
fclose(f);
printf("buf: 0x%lx, sz: %lu, errno: %d\n",
(unsigned long) buf, sz, errno);
return 0;
}
```
```
$ gcc -o main main.c
$ ./main
buf: 0x74d4a0, sz: 5, errno: 0
buf: 0x0, sz: 5, errno: 0
```
This might do unexpected things if the underlying code expects a valid
pointer to the memstream buffer after closing the handle.
Found by Nallocfuzz.
This allows a double-click on the path in a terminal to select the
whole path. Otherwise the leading '-' character is also included in
the copied path.
```
New output:
./busctl tree org.freedesktop.network1
`- /org
`- /org/freedesktop
|- /org/freedesktop/LogControl1
`- /org/freedesktop/network1
|- /org/freedesktop/network1/link
| |- /org/freedesktop/network1/link/_31
| |- /org/freedesktop/network1/link/_32
```
Unlike most other bus connections in our codebase this one is created
manually and every setting set invididually. It hence does not have a
description by default (as all automatic connections have). Set one
explicitly.
In various tools and services we have a per-system and per-user concept.
So far we sometimes used a boolean indicating whether we are in system
mode, or a reversed boolean indicating whether we are in user mode, or
the LookupScope enum used by the lookup path logic.
Let's address that, in introduce a common enum for this, we can use all
across the board.
This is mostly just search/replace, no actual code changes.
Although this slightly more verbose it makes it much easier to reason
about. The code that produces the tests heavily benefits from this.
Test lists are also now sorted by test name.
All users were setting this to some static string (usually "-"), so let's
simplify things by not doing strdup, but instead limiting callers to a fixed
set of values. In preparation for the next commit, the function is renamed from
"empty" to "replacement", because it'll be used for more than empty fields. I
didn't do the whole string-table setup, because it's all used internally in one
file and this way we can immediately assert if an invalid value is passed in.
Some callers were (void)ing the error, others were ignoring it, and others
propagating. It's nicer to remove the boilerplate.
D-Bus interfaces can have multiple methods with the same name, as long
as they have different arguments (signature). Currently busctl can call
those methods but when introspecting the interface it just displays
"Duplicate method"
This PR fixes the behavior, by also adding the signature to the hash for
the members set.
Before this patch:
$ busctl introspect org.asamk.Signal /org/asamk/Signal
Invalid introspection data: duplicate method 'sendMessage' on interface 'org.asamk.Signal'.
After this patch:
$ busctl introspect org.asamk.Signal /org/asamk/Signal
NAME TYPE SIGNATURE RESULT/VALUE FLAGS
org.asamk.Signal interface - - -
.sendMessage method as x -
.sendMessage method s x -
Calling the methods already works as expected, as the user must specify
the signature explicitely:
busctl --user call org.asamk.Signal /org/asamk/Signal org.asamk.Signal sendMessage "as" 2 foo bar
busctl --user call org.asamk.Signal /org/asamk/Signal org.asamk.Signal sendMessage "s" foo
$ busctl --xml introspect org.asamk.Signal /org/asamk/Signal
<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" "http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd">
<node name="/org/asamk/Signal">
<interface name="org.asamk.Signal">
<method name="sendMessage" >
<arg type="as" direction="in"/>
<arg type="x" direction="out"/>
</method>
<method name="sendMessage" >
<arg type="s" direction="in"/>
<arg type="x" direction="out"/>
</method>
<interface name="org.freedesktop.DBus.Introspectable">
<method name="Introspect">
<arg type="s" direction="out"/>
</method>
</interface>
<interface name="org.freedesktop.DBus.Peer">
<method name="Ping">
</method>
</interface>
</node>
This patch changes busctl capture to generate pcapng format
instead of the legacy pcap format files. It includes basic
meta-data in the file and still uses microsecond time
resolution. In future, more things can be added such as
high resolution timestams, statistics, etc.
PCAP Next Generation capture file format is what tshark uses
and is in process of being standardized in IETF. It is also
readable with libpcap.
$ capinfos /tmp/new.pcapng
File name: /tmp/new.pcapng
File type: Wireshark/... - pcapng
File encapsulation: D-Bus
File timestamp precision: microseconds (6)
Packet size limit: file hdr: (not set)
Packet size limit: inferred: 4096 bytes
Number of packets: 22
File size: 21kB
Data size: 20kB
Capture duration: 0.005694 seconds
First packet time: 2021-12-11 11:57:42.788374
Last packet time: 2021-12-11 11:57:42.794068
Data byte rate: 3,671kBps
Data bit rate: 29Mbps
Average packet size: 950.27 bytes
Average packet rate: 3,863 packets/s
SHA256: b85ed8b094af60c64aa6d9db4a91404e841736d36b9e662d707db9e4096148f1
RIPEMD160: 81f9bac7ec0ec5cd1d55ede136a5c90413894e3a
SHA1: 8400822ef724b934d6000f5b7604b9e6e91be011
Strict time order: True
Capture oper-sys: Linux 5.14.0-0.bpo.2-amd64
Capture application: systemd 250 (250-rc2-33-gdc79ae2+)
Number of interfaces in file: 1
Interface #0 info:
Encapsulation = D-Bus (146 - dbus)
Capture length = 4096
Time precision = microseconds (6)
Time ticks per second = 1000000
Number of stat entries = 0
Number of packets = 22
(Or when -H is used, since -H and -M are incompatible.)
Note that the slightly unusual form with separate boolean variables (hint_vars,
hint_addr) instead of e.g. a const char* variable to hold the message, because this
way we don't trigger the warning about non-literal format.
These functions are used pretty much independently of locale, i.e. the
only info relevant is whether th locale is UTF-8 or not. Hence let's
give this its own pair of .c/.h files.
In general we almost never hit those asserts in production code, so users see
them very rarely, if ever. But either way, we just need something that users
can pass to the developers.
We have quite a few of those asserts, and some have fairly nice messages, but
many are like "WTF?" or "???" or "unexpected something". The error that is
printed includes the file location, and function name. In almost all functions
there's at most one assert, so the function name alone is enough to identify
the failure for a developer. So we don't get much extra from the message, and
we might just as well drop them.
Dropping them makes our code a tiny bit smaller, and most importantly, improves
development experience by making it easy to insert such an assert in the code
without thinking how to phrase the argument.
We recently started making more use of malloc_usable_size() and rely on
it (see the string_erase() story). Given that we don't really support
sytems where malloc_usable_size() cannot be trusted beyond statistics
anyway, let's go fully in and rework GREEDY_REALLOC() on top of it:
instead of passing around and maintaining the currenly allocated size
everywhere, let's just derive it automatically from
malloc_usable_size().
I am mostly after this for the simplicity this brings. It also brings
minor efficiency improvements I guess, but things become so much nicer
to look at if we can avoid these allocation size variables everywhere.
Note that the malloc_usable_size() man page says relying on it wasn't
"good programming practice", but I think it does this for reasons that
don't apply here: the greedy realloc logic specifically doesn't rely on
the returned extra size, beyond the fact that it is equal or larger than
what was requested.
(This commit was supposed to be a quick patch btw, but apparently we use
the greedy realloc stuff quite a bit across the codebase, so this ends
up touching *a*lot* of code.)
With some versions of the compiler, the _cleanup_ attr makes it think
the variable might be freed/closed when uninitialized, even though it
cannot happen. The added cost is small enough to be worth the benefit,
and optimized builds will help reduce it even further.
