==16725==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000005a159f at pc 0x00000046fc1b bp 0x7fff6038f900 sp 0x7fff6038f8f0
READ of size 1 at 0x0000005a159f thread T0
#0 0x46fc1a in _do_test_unescape_spaces libnm-core/tests/test-general.c:7791
#1 0x46fe5b in test_nm_utils_unescape_spaces libnm-core/tests/test-general.c:7810
#2 0x7f4ac5fe7fc9 in test_case_run gtestutils.c:2318
#3 0x7f4ac5fe7fc9 in g_test_run_suite_internal gtestutils.c:2403
#4 0x7f4ac5fe7e83 in g_test_run_suite_internal gtestutils.c:2415
#5 0x7f4ac5fe7e83 in g_test_run_suite_internal gtestutils.c:2415
#6 0x7f4ac5fe8281 in g_test_run_suite gtestutils.c:2490
#7 0x7f4ac5fe82a4 in g_test_run (/lib64/libglib-2.0.so.0+0x772a4)
#8 0x48240d in main libnm-core/tests/test-general.c:7994
#9 0x7f4ac5dc9412 in __libc_start_main (/lib64/libc.so.6+0x24412)
#10 0x423ffd in _start (/home/bgalvani/work/NetworkManager/libnm-core/tests/test-general+0x423ffd)
0x0000005a159f is located 49 bytes to the right of global variable '*.LC370' defined in 'libnm-core/tests/test-general.c' (0x5a1560) of size 14
'*.LC370' is ascii string 'nick-5, green'
0x0000005a159f is located 1 bytes to the left of global variable '*.LC371' defined in 'libnm-core/tests/test-general.c' (0x5a15a0) of size 1
'*.LC371' is ascii string ''
SUMMARY: AddressSanitizer: global-buffer-overflow libnm-core/tests/test-general.c:7791 in _do_test_unescape_spaces
Leak detection adds unhelpful messages to the stderr of nmcli, making
tests fail. For example:
=================================================================
==17156==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 256 byte(s) in 2 object(s) allocated from:
#0 0x7f08c7e27c88 in realloc (/lib64/libasan.so.5+0xefc88)
#1 0x7f08c7546e7d in g_realloc (/lib64/libglib-2.0.so.0+0x54e7d)
We already have code that parses exactly this kinds of string:
nm_utils_parse_inaddr_prefix_bin(). Use it.
Also, it doesn't use g_strsplit_set() to separate a string at the first
'/'. Total overkill.
- use cleanup macros everywhere.
- In particular use nm_auto_clear_variant_builder to free the
GVariantBuilder in the error cases. Note that the error cases
anyway are asserted against, so during a normal test run there
was no leak. But we should not write software like that.
- use nm_utils_strsplit_set_with_empty() instead of g_strsplit_set().
We should use our variant also in unit-tests, because that way the
function gets more test coverage. And it likely performs better
anyway.
Currently, if user configuration or settings specify that a software
device is unmanaged, for example:
[device-bond-unmanaged]
match-device=interface-name:bond*
managed=0
or
[keyfile]
unmanaged-devices=interface-name:bond*
and there is a connection for the device with autoconnect=yes, NM
creates the platform link and a realized device in unmanaged
state. Fix this, the device should not be realized if it is unmanaged.
https://bugzilla.redhat.com/show_bug.cgi?id=1679230
nm_device_spec_match_list_full() calls
nm_device_get_permanent_hw_address() which freezes the MAC address, so
currently callers must avoid the function when the device is not
completely platform-initialized.
Instead, use nm_device_get_permanent_hw_address_full() to avoid
freezing the MAC when the device is not platform-initialized. In this
way nm_device_spec_match_list_full() can be called from any state
without side effects.
Instead of growing the buffer for the tokens (and reallocating),
do one pre-run over the string and count the delimiters. This
way we know how much space we need and we don't need to
reallocate.
Interestingly, this is notably slower than the previous implementation,
because previously if would not bother determining the right number of
tokens but just over-allocate with a reasonable guess of 8 and grow the
buffer exponentially. Still, I like this better because while it may
be slower in common scenarios, it allocates the exact number of buffer
space.
- use nm_utils_strsplit_set_full() instead of g_strsplit_set() to avoid allocating
a full strv array.
- refactor the code to return early and use cleanup attribute for freeing
memory.
- return TRUE/FALSE from process_dhclient_rfc3442_route(). It's simpler to
understand than returning the moved pointer and a success output variable.
Previously, nm_utils_strsplit_set_full() would always remove empty
tokens. Add a flag NM_UTILS_STRSPLIT_SET_FLAGS_PRESERVE_EMPTY to avoid
that.
This makes nm_utils_strsplit_set_full() return the same result as
g_strsplit_set() and a direct replacement for it -- except for "",
where we return %NULL.
Drop the next_char() and is_delimiter() macros. They are difficult to
understand, because they both have a state-variable (escaped).
Instead, the state of whether we handle an escape or not, shall only
depend on the current line of code.
The caller should make a conscious decision which delimiters to use.
Unfortunately, there is a variety of different demiters in use. This
should be unitfied and the callers should use one of a few specific
set of delimiters.
This could be unified by (re)using a define as delimiters, like
strv = nm_utils_strsplit_set_full (value, MULTILIST_WITH_ESCAPE_CHARS, NM_UTILS_STRSPLIT_SET_FLAGS_ALLOW_ESCAPING);
where MULTILIST_WITH_ESCAPE_CHARS has a particular meaning that should
be reused for similar uses.
However, leaving the delimiter at NULL is not good because it's unclear who
wants that default behavior (and what the default should be). Don't allow that.
There are almost no callers that relied on this default anyway.
If NM fails to connect to teamd, it currently just sets the device
state to FAILED and waits that deactivate() is called later. However,
the 5 seconds timeout on teamd process start can hit in the meantime,
which fails with an assertion "nm_device_is_activating (device)".
Clean up the device state when the connection to teamd fails.
https://bugzilla.redhat.com/show_bug.cgi?id=1697900
We call GetConnectionUnixProcessID and GetConnectionUnixUser *a lot*.
And we do so synchronously. Both is a problem.
To avoid the first problem, cache the last few requests with each cached
value being valid for one second.
On a quick test, this saves 98% of the requests:
59 GetConnectionUnixProcessID(*)
3201 GetConnectionUnixProcessID(*) (served from cache)
59 GetConnectionUnixUser(*)
3201 GetConnectionUnixUser(*) (served from cache)
Note that now as we serve requests from the cache, it might be the case
that the D-Bus endpoint already disconnected. Previously, the request would
have failed but now we return the cached user-id and process-id. This
problem is mitigated by only caching the values for up to one second.
Also, it's not really a problem because we cache sender names. Those
are supposed to be unique and not repeat. So, even if the peer already
disconnected, it is still true that the corresponding PID/UID was as
we have cached it. We don't use this API for checking whether the peer
is still connected, but what UID/PID it has/had. That answer is still
correct for the cached value after the peer disconnected.
The proxy does nothing for us, except overhead.
We can directly subscribe to "NameOwnerChanged" signals on the
GDBusConnection. Also, instead of asynchronously creating the
GDBusProxy, asynchronously call "GetNameOwner". That's what the
proxy does anyway.
GDBusConnection is actually a decent API. We don't need another layer on
top of that, for functionality that we don't use.
Also, don't use G_BUS_TYPE_SYSTEM, but use the GDBusConnection that
also the bus-manager uses. For all practical purposes, that is the
connection was want to use also in NMDnsSystemdResolved.
