The leak can be reproduced by running systemd-path --suffix .tmp under valgrind or asan:
$ ./build/systemd-path --suffix .tmp search-binaries
/usr/local/bin/.tmp:/usr/bin/.tmp:/usr/local/sbin/.tmp:/usr/sbin/.tmp:/home/vagrant/.local/bin/.tmp:/home/vagrant/bin/.tmp
=================================================================
==19177==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 56 byte(s) in 1 object(s) allocated from:
*0 0x7fd6adf72850 in malloc (/lib64/libasan.so.4+0xde850)
*1 0x7fd6ad2b93d2 in malloc_multiply ../src/basic/alloc-util.h:69
*2 0x7fd6ad2bafd2 in strv_split ../src/basic/strv.c:269
*3 0x7fd6ad42ba67 in search_from_environment ../src/libsystemd/sd-path/sd-path.c:409
*4 0x7fd6ad42bffe in get_search ../src/libsystemd/sd-path/sd-path.c:482
*5 0x7fd6ad42c55b in sd_path_search ../src/libsystemd/sd-path/sd-path.c:607
*6 0x7fd6ad42b3a2 in sd_path_home ../src/libsystemd/sd-path/sd-path.c:348
*7 0x55f59c65ebea in print_home ../src/path/path.c:97
*8 0x55f59c65f157 in main ../src/path/path.c:177
*9 0x7fd6abaea009 in __libc_start_main (/lib64/libc.so.6+0x21009)
Indirect leak of 68 byte(s) in 5 object(s) allocated from:
*0 0x7fd6adf72850 in malloc (/lib64/libasan.so.4+0xde850)
*1 0x7fd6abb5f689 in strndup (/lib64/libc.so.6+0x96689)
Indirect leak of 25 byte(s) in 1 object(s) allocated from:
*0 0x7fd6adf72850 in malloc (/lib64/libasan.so.4+0xde850)
*1 0x7fd6abb5f689 in strndup (/lib64/libc.so.6+0x96689)
*2 0x6c2e2f746e617266 (<unknown module>)
SUMMARY: AddressSanitizer: 149 byte(s) leaked in 7 allocation(s).
Previously we were a bit sloppy with the index and size types of arrays,
we'd regularly use unsigned. While I don't think this ever resulted in
real issues I think we should be more careful there and follow a
stricter regime: unless there's a strong reason not to use size_t for
array sizes and indexes, size_t it should be. Any allocations we do
ultimately will use size_t anyway, and converting forth and back between
unsigned and size_t will always be a source of problems.
Note that on 32bit machines "unsigned" and "size_t" are equivalent, and
on 64bit machines our arrays shouldn't grow that large anyway, and if
they do we have a problem, however that kind of overly large allocation
we have protections for usually, but for overflows we do not have that
so much, hence let's add it.
So yeah, it's a story of the current code being already "good enough",
but I think some extra type hygiene is better.
This patch tries to be comprehensive, but it probably isn't and I missed
a few cases. But I guess we can cover that later as we notice it. Among
smaller fixes, this changes:
1. strv_length()' return type becomes size_t
2. the unit file changes array size becomes size_t
3. DNS answer and query array sizes become size_t
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=76745
This cleans up handling of MTU values across the codebase. Previously
MTU values where stored sometimes in uint32_t, sometimes in uint16_t,
sometimes unsigned and sometimes in size_t. This now unifies this to
uint32_t across the codebase, as that's what netlink spits out, and what
the majority was already using.
Also, all MTU parameters are now parsed with config_parse_mtu() and
config_parse_ipv6_mtu() is dropped as it is now unneeded.
(Note there is one exception for the MTU typing: in the DCHPv4 code we
continue to process the MTU as uint16_t value, as it is encoded like
that in the protocol, and it's probably better stay close to the
protocol there.)
Double newlines (i.e. one empty lines) are great to structure code. But
let's avoid triple newlines (i.e. two empty lines), quadruple newlines,
quintuple newlines, …, that's just spurious whitespace.
It's an easy way to drop 121 lines of code, and keeps the coding style
of our sources a bit tigther.
Files which are installed as-is (any .service and other unit files, .conf
files, .policy files, etc), are left as is. My assumption is that SPDX
identifiers are not yet that well known, so it's better to retain the
extended header to avoid any doubt.
I also kept any copyright lines. We can probably remove them, but it'd nice to
obtain explicit acks from all involved authors before doing that.
sd_bus_open/sd_bus_open_system/sd_bus_open_user are convenient, but
don't allow the description to be set. After they return, the bus is
is already started, and sd_bus_set_description() fails with -EBUSY.
It would be possible to allow sd_bus_set_description() to update the
description "live", but messages are already emitted from sd_bus_open
functions, so it's better to allow the description to be set in
sd_bus_open/sd_bus_open_system/sd_bus_open_user.
Fixes message like:
Bus n/a: changing state UNSET → OPENING
This macro will read a pointer of any type, return it, and set the
pointer to NULL. This is useful as an explicit concept of passing
ownership of a memory area between pointers.
This takes inspiration from Rust:
https://doc.rust-lang.org/std/option/enum.Option.html#method.take
and was suggested by Alan Jenkins (@sourcejedi).
It drops ~160 lines of code from our codebase, which makes me like it.
Also, I think it clarifies passing of ownership, and thus helps
readability a bit (at least for the initiated who know the new macro)
Some versions of asan report the following false positive
when strict_string_checks=1 is passed:
=================================================================
==3297==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f64e4090286 bp 0x7ffe46acd9a0 sp 0x7ffe46acd118 T0)
==3297==The signal is caused by a READ memory access.
==3297==Hint: address points to the zero page.
#0 0x7f64e4090285 in __strlen_sse2 (/lib64/libc.so.6+0xaa285)
#1 0x7f64e5a51e46 (/lib64/libasan.so.4+0x41e46)
#2 0x7f64e4e5e3a0 (/lib64/libglib-2.0.so.0+0x383a0)
#3 0x7f64e4e5e536 in g_dgettext (/lib64/libglib-2.0.so.0+0x38536)
#4 0x7f64e48fac5f (/lib64/libgio-2.0.so.0+0xc1c5f)
#5 0x7f64e4c03978 in g_type_class_ref (/lib64/libgobject-2.0.so.0+0x30978)
#6 0x7f64e4be9567 in g_object_new_with_properties (/lib64/libgobject-2.0.so.0+0x16567)
#7 0x7f64e4be9fd0 in g_object_new (/lib64/libgobject-2.0.so.0+0x16fd0)
#8 0x7f64e48fd43e in g_dbus_message_new_from_blob (/lib64/libgio-2.0.so.0+0xc443e)
#9 0x564a6aa0de52 in main ../src/libsystemd/sd-bus/test-bus-marshal.c:228
#10 0x7f64e4007009 in __libc_start_main (/lib64/libc.so.6+0x21009)
#11 0x564a6aa0a569 in _start (/home/vagrant/systemd/build/test-bus-marshal+0x5569)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib64/libc.so.6+0xaa285) in __strlen_sse2
==3297==ABORTING
It's an external library and errors in external libraries are generally not very
useful for looking for internal bugs.
It would be better not to change the code and use standard suppression
techinques decribed at
https://clang.llvm.org/docs/AddressSanitizer.html#suppressing-reports-in-external-libraries,
but, unfortunaley, none of them seems to be able to suppress fatal errors in asan intself.
There isn't much difference, but in general we prefer to use the standard
functions. glibc provides reallocarray since version 2.26.
I moved explicit_bzero is configure test to the bottom, so that the two stdlib
functions are at the bottom.
Let's make use this at various places we call fsync(), to make things
fully reliable, as the kernel devs suggest to first fsync() files and
then fsync() the directories they are located in.
This adds some paranoia code that moves some of the fds we allocate for
longer periods of times to fds > 2 if they are allocated below this
boundary. This is a paranoid safety thing, in order to avoid that
external code might end up erroneously use our fds under the assumption
they were valid stdin/stdout/stderr. Think: some app closes
stdin/stdout/stderr and then invokes 'fprintf(stderr, …' which causes
writes on our fds.
This both adds the helper to do the moving as well as ports over a
number of users to this new logic. Since we don't want to litter all our
code with invocations of this I tried to strictly focus on fds we keep
open for long periods of times only and only in code that is frequently
loaded into foreign programs (under the assumptions that in our own
codebase we are smart enough to always keep stdin/stdout/stderr
allocated to avoid this pitfall). Specifically this means all code used
by NSS and our sd-xyz API:
1. our logging APIs
2. sd-event
3. sd-bus
4. sd-resolve
5. sd-netlink
This changed was inspired by this:
https://github.com/systemd/systemd/issues/8075#issuecomment-363689755
This shows that apparently IRL there are programs that do close
stdin/stdout/stderr, and we should accomodate for that.
Note that this won't fix any bugs, this just makes sure that buggy
programs are less likely to interfere with out own code.
we still invoke ssh unnecessarily when there in incompatible or erreneous input
The fallow-up to finish that would make the code a bit more verbose,
as it would require repeating this bit:
```
r = bus_connect_transport(arg_transport, arg_host, false, &bus);
if (r < 0) {
log_error_errno(r, "Failed to create bus connection: %m");
goto finish;
}
sd_bus_set_allow_interactive_authorization(bus, arg_ask_password);
```
in every verb, after parsing.
v2: add waitpid() to avoid a zombie process, switch to SIGTERM from SIGKILL
v3: refactor, wait in bus_start_address()
Zbigniew Jędrzejewski-Szmek
Yu Watanabe
David Tardon
Davide Cavalca
Filipe Brandenburger
Lennart Poettering
Daniel Dao
Evegeny Vereshchagin
Davide Cavalca
Shawn Landden