Currently the only way to return an error code from the daemon helper
is via the process exit code, but that is not enough to fully describe
an error from getaddrinfo(); in fact, the function returns a EAI_*
error code and when the value is EAI_SYSTEM, the error code is
returned in errno.
At the moment, any messages printed to stderr by the helper goes to NM
stderr; instead, we want to capture it and pass it through the logging
mechanism of NM, so that it can be filtered according to level and
domain.
g_random_*() is based on GRand, which is not a CSPRNG. Instead, rely on
kernel to give us good random numbers, which is what nm_random_*() does.
Note that nm_random_*() calls getrandom() (or reads /dev/urandom), which
most likely is slower than GRand. It doesn't matter for our uses though.
It is cumbersome to review all uses of g_rand_*() whether their usage of
a non-cryptographically secure generator is appropriate. Instead, just
always use an appropriate function, thereby avoiding this question. Even
glib documentation refers to reading "/dev/urandom" as alternative. Which
is what nm_random_*() does. These days, it seems unnecessary to not use
the best random generator available, unless it's not fast enough or you
need a stable/seedable stream of random numbers.
In particular in nmcli, we used g_random_int_range() to generate
passwords. That is not appropriate. Sure, it's *only* for the hotspot,
but still.
nm_utils_get_ipv6_interface_identifier() has non-obvious requirements on
the hardware address. If the caller passes a wrong length, it will
trigger an assertion or even cause out of bound read. This would mean
that the caller needs to carefully check the length. Such requirements
on the caller are wrong.
Also, in practice the hardware length comes from platform/kernel. We
don't want to trust that what kernel tells us always has the required
address length, so the caller would always have to double check before
calling the function.
Instead, handle unexpected address lengths.
Fixes: e2270040c0 ('core: use Interface Identifiers for IPv6 SLAAC addresses')
Fixes: 1d396e9972 ('core-utils: use 64-bit WPAN address for a 6LoWPAN IID')
This is the version shipped in Fedora 37. As Fedora 37 is now out, the
core developers switch to it. Our gitlab-ci will also use that as base
image for the check-{patch.tree} tests and to generate the pages. There
is a need that everybody agrees on which clang-format version to use,
and that version should be the one of the currently used Fedora release.
Also update the used Fedora image in "contrib/scripts/nm-code-format-container.sh"
script.
The gitlab-ci still needs update in the following commit. The change
in isolation will break the "check-tree" test.
Clang 15 now (correctly) warns about this:
../src/libnm-core-impl/nm-vpn-plugin-info.c:201:40: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
_nm_vpn_plugin_info_get_default_dir_etc()
^
void
../src/libnm-core-impl/nm-vpn-plugin-info.c:213:40: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
_nm_vpn_plugin_info_get_default_dir_lib()
^
void
../src/libnm-core-impl/nm-vpn-plugin-info.c:226:41: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
_nm_vpn_plugin_info_get_default_dir_user()
^
void
../src/libnm-core-impl/nm-vpn-plugin-info.c:315:29: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
nm_vpn_plugin_info_list_load()
^
void
These variants provide additional nm_assert() checks, and are thus
preferable.
Note that we cannot just blindly replace &g_array_index() with
&nm_g_array_index(), because the latter would not allow getting a
pointer at index [arr->len]. That might be a valid (though uncommon)
usecase. The correct replacement of &g_array_index() is thus
nm_g_array_index_p().
I checked the code manually and replaced uses of nm_g_array_index_p()
with &nm_g_array_index(), if that was a safe thing to do. The latter
seems preferable, because it is familar to &g_array_index().
Add nm_g_array_index() as a replacement for g_array_index(). The value
of nm_g_array_index(), nm_g_array_index_p(), nm_g_array_first() and
nm_g_array_last() is that they add nm_assert() checks for valid
parameters.
nm_g_array_{first,last}() now returns an lvalue and not a pointer.
As such, they are just shorthands for nm_g_array_index() at index
0 and len-1, respectively.
`nm_g_array_index_p(arr, Type, idx)` is almost the same as
`&nm_g_array_index(arr, Type, idx)`. The only difference (and why the
former variant exists), is that nm_g_array_index_p() allows to get a
pointer one after the end.
This means, this is correct and valid to do:
// arr->len might be zero
arr = nm_g_array_index_p(arr, Type, 0);
for (i = 0; i < arr->len; i++, arr++)
...
ptr = nm_g_array_index_p(arr, Type, 0);
end = nm_g_array_index_p(arr, Type, arr->len);
for (; ptr < end; ptr++)
...
This would not be valid to do with nm_g_array_{index,first,last}().
Also fix supporting "const GArray *arr" parameter. Of course, the function
casts the constness away. Technically, that matches the fact that arr->data
is also not a const pointer. In practice, we might want to propagate the
constness of the container to the constness of the element lookup. While
doable, that is not implemented.
- name things related to `in_addr_t`, `struct in6_addr`, `NMIPAddr` as
`nm_ip4_addr_*()`, `nm_ip6_addr_*()`, `nm_ip_addr_*()`, respectively.
- we have a wrapper `nm_inet_ntop()` for `inet_ntop()`. This name
of our wrapper is chosen to be familiar with the libc underlying
function. With this, also name functions that are about string
representations of addresses `nm_inet_*()`, `nm_inet4_*()`,
`nm_inet6_*()`. For example, `nm_inet_parse_str()`,
`nm_inet_is_normalized()`.
<<<<
R() {
git grep -l "$1" | xargs sed -i "s/\<$1\>/$2/g"
}
R NM_CMP_DIRECT_IN4ADDR_SAME_PREFIX NM_CMP_DIRECT_IP4_ADDR_SAME_PREFIX
R NM_CMP_DIRECT_IN6ADDR_SAME_PREFIX NM_CMP_DIRECT_IP6_ADDR_SAME_PREFIX
R NM_UTILS_INET_ADDRSTRLEN NM_INET_ADDRSTRLEN
R _nm_utils_inet4_ntop nm_inet4_ntop
R _nm_utils_inet6_ntop nm_inet6_ntop
R _nm_utils_ip4_get_default_prefix nm_ip4_addr_get_default_prefix
R _nm_utils_ip4_get_default_prefix0 nm_ip4_addr_get_default_prefix0
R _nm_utils_ip4_netmask_to_prefix nm_ip4_addr_netmask_to_prefix
R _nm_utils_ip4_prefix_to_netmask nm_ip4_addr_netmask_from_prefix
R nm_utils_inet4_ntop_dup nm_inet4_ntop_dup
R nm_utils_inet6_ntop_dup nm_inet6_ntop_dup
R nm_utils_inet_ntop nm_inet_ntop
R nm_utils_inet_ntop_dup nm_inet_ntop_dup
R nm_utils_ip4_address_clear_host_address nm_ip4_addr_clear_host_address
R nm_utils_ip4_address_is_link_local nm_ip4_addr_is_link_local
R nm_utils_ip4_address_is_loopback nm_ip4_addr_is_loopback
R nm_utils_ip4_address_is_zeronet nm_ip4_addr_is_zeronet
R nm_utils_ip4_address_same_prefix nm_ip4_addr_same_prefix
R nm_utils_ip4_address_same_prefix_cmp nm_ip4_addr_same_prefix_cmp
R nm_utils_ip6_address_clear_host_address nm_ip6_addr_clear_host_address
R nm_utils_ip6_address_same_prefix nm_ip6_addr_same_prefix
R nm_utils_ip6_address_same_prefix_cmp nm_ip6_addr_same_prefix_cmp
R nm_utils_ip6_is_ula nm_ip6_addr_is_ula
R nm_utils_ip_address_same_prefix nm_ip_addr_same_prefix
R nm_utils_ip_address_same_prefix_cmp nm_ip_addr_same_prefix_cmp
R nm_utils_ip_is_site_local nm_ip_addr_is_site_local
R nm_utils_ipaddr_is_normalized nm_inet_is_normalized
R nm_utils_ipaddr_is_valid nm_inet_is_valid
R nm_utils_ipx_address_clear_host_address nm_ip_addr_clear_host_address
R nm_utils_parse_inaddr nm_inet_parse_str
R nm_utils_parse_inaddr_bin nm_inet_parse_bin
R nm_utils_parse_inaddr_bin_full nm_inet_parse_bin_full
R nm_utils_parse_inaddr_prefix nm_inet_parse_with_prefix_str
R nm_utils_parse_inaddr_prefix_bin nm_inet_parse_with_prefix_bin
R test_nm_utils_ip6_address_same_prefix test_nm_ip_addr_same_prefix
./contrib/scripts/nm-code-format.sh -F
_host_id_read() is the only place where we really care to have good
random numbers, because that is the secret key that we persist to disk.
Previously, we tried only nm_random_get_bytes_full(), which is a best
effort to get strong random numbers. If it fails to generate those,
it would simply remember the generated key in memory and proceed, but not
persist it to disk.
nm_random_get_bytes_full() does not block waiting for good numbers.
Change that. Now, first call nm_random_get_crypto_bytes(), which would
block and try hard to get good random numbers. Only if that fails,
fallback to nm_random_get_bytes_full() as before. The difference is of
course only in early boot, when we might not yet have entropy. In that
case, I think it's better for NetworkManager to block.
Heavily inspired by systemd ([1]).
We now also have nm_random_get_bytes{,_full}() and
nm_random_get_crypto_bytes(), like systemd's random_bytes()
and crypto_random_bytes(), respectively.
Differences:
- instead of systemd's random_bytes(), our nm_random_get_bytes_full()
also estimates whether the output is of high quality. The caller
may find that interesting. Due to that, we will first try to call
getrandom(GRND_NONBLOCK) before getrandom(GRND_INSECURE). That is
reversed from systemd's random_bytes(), because we want to find
out whether we can get good random numbers. In most cases, kernel
should have entropy already, and it makes no difference.
Otherwise, heavily rework the code. It should be easy to understand
and correct.
There is also a major bugfix here. Previously, if getrandom() failed
with ENOSYS and we fell back to /dev/urandom, we would assume that we
have high quality random numbers. That assumption is not warranted.
Now instead poll on /dev/random to find out.
[1] a268e7f402/src/basic/random-util.c (L81)
We read /proc/cmdline for "match.kernel-command-line". But next we will
also honor "nm.debug" on the kernel command line, to enable debug
logging. For "nm.debug" it makes sense that it overwrites the debug
options from the command line and from "NetworkManager.conf". That
means, if you set "nm.debug", then verbose logging will be enabled. It
can only be turned off again at runtime (via D-Bus), otherwise, it's
hard to avoid.
It still can make sense to overrule this setting once again. Support
that, by honoring a file "/run/NetworkManager/proc-cmdline" to be used
instead of "/proc/cmdline".
This option is mainly for debugging and testing, but it might be useful
in production too, if you had "nm.debug" enabled during boot, but later
want to disable it until next reboot. Then you could do:
sed 's/ *\<nm\.debug\> */ /g' /proc/cmdline > /run/NetworkManager/proc-cmdline
nmcli general logging level DEFAULT domains DEFAULT
NM_STR_BUF_INIT() and nm_str_buf_init() were pretty much redundant. Drop one of
them.
Usually our pattern is that we don't have functions that return structs.
But NM_STR_BUF_INIT() returns a struct, because it's convenient to use
with
nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(...);
So use that variant instead.
On glibc, HOST_NAME_MAX is defined as 64. Also, Linux'
sethostname() enforces that limit (__NEW_UTS_LEN). Also,
`man gethostname` comments that HOST_NAME_MAX on Linux is
64.
However, when building against musl, HOST_NAME_MAX is defined as 255.
That seems wrong. We use this limit to validate the hostname, and that
should not depend on the libc or on the compilation.
Hardcode the value to 64.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1197
While NetworkManager is of course (mostly) single threaded,
our static functions really should be thread safe.
"mostly" single threaded because we have GDBus's worker
thread, we use a thread for writing non-blocking SR-IOV sysctl,
in the past (or still?) we used a thread for async glibc resolver.
Anyway, a low-level function like must be thread safe, when it
uses global data.
Granted, the initialize-once pattern with the flag and the
int variable, is probably in many cases good enough. But it
makes me unhappy, the thought of accessing static data without
a synchronized operation.
There is a hierarchy of how files include each other. "main-utils.h"
is pretty much at the bottom (one above "main.c"), in the sense that
it only includes other headers, but is not included itself (aside
"main.c").
Move the utils function to a place where its accessible from everywhere
and rename.
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
The host-id gets read from /var/lib/NetworkManager/secret_key, and cached in
a global variable. Other parts of the code can get the host ID using a
singleton function.
For testing, we need to inject a different host-id. Add two push/pop
functions for that.
Unlike nm_utils_host_id_get(), these functions are not thread-safe (nor
is it possible to make them thread-safe in a reasonable manner).
It's great to have functions that cannot fail, because it allows to
skip any error handling.
_set_stable_privacy() as it was could not fail, so the only reason why
nm_utils_ipv6_addr_set_stable_privacy() could fail is because the DAD
counter exhausted.
Also, it will be useful to have a function that does not do the counter
check, where the caller wants to handle that differently.
Rename some functions, and make the core nm_utils_ipv6_addr_set_stable_privacy()
not failable.
The "utils" part does not seem useful in the name.
Note that we also have NMStrBuf, which is named nm_str_buf_*().
There is an unfortunate similarity between the two, but it's still
distinct enough (in particular, because one takes an NMStrBuf and
the other not).
Add function nm_utils_connection_match_spec_list() to check whether a
connection matches a spec list. Also document the supported syntax in
the man page.
The user really must not treat UuidData.bin as a NUL terminated string.
The _nul_sentinel is not necessary. And if by chance the user makes this
fatal mistake, then UuidData.str will still be there to NUL terminate the
buffer, the content is garbage either way.
Remove the sentinel.
nm_uuid_generate_from_string*() accepts an optional namespace parameter,
to seed the hashing. This previously was a UUID in string format, so it
first had to be parsed.
Rework the code to pass a NMUuid instance that can be used directly.
Also, as the type_args parameter is always of the same type, change
the argument from a void pointer to "const NMUuid *" pointer.
Extend nm_utils_file_set_contents to be able to optionally set the last
access + last modification times on the file being created, in addition
to the mode.
It's not entirely clear how to treat %NULL.
Clearly "match.interface-name=eth0" should not
match with an interface %NULL. But what about
"match.interface-name=!eth0"? It's now implemented
that negative matches still succeed against %NULL.
What about "match.interface-name=*"? That probably
should also match with %NULL. So we treat %NULL really
like "".
Against commit 11cd443448 ('iwd: Don't call IWD methods when device
unmanaged'), we got this backtrace:
#0 0x00007f1c164069f1 in __strnlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:62
#1 0x00007f1c1637ac9e in __fnmatch (pattern=<optimized out>, string=<optimized out>, string@entry=0x0, flags=flags@entry=0) at fnmatch.c:379
p = 0x0
res = <optimized out>
orig_pattern = <optimized out>
n = <optimized out>
wpattern = 0x7fff8d860730 L"pci-0000:03:00.0"
ps = {__count = 0, __value = {__wch = 0, __wchb = "\000\000\000"}}
wpattern_malloc = 0x0
wstring_malloc = 0x0
wstring = <optimized out>
alloca_used = 80
__PRETTY_FUNCTION__ = "__fnmatch"
#2 0x0000564484a978bf in nm_wildcard_match_check (str=0x0, patterns=<optimized out>, num_patterns=<optimized out>) at src/core/nm-core-utils.c:1959
is_inverted = 0
is_mandatory = 0
match = <optimized out>
p = 0x564486c43fa0 "pci-0000:03:00.0"
has_optional = 0
has_any_optional = 0
i = <optimized out>
#3 0x0000564484bf4797 in check_connection_compatible (self=<optimized out>, connection=<optimized out>, error=0x0) at src/core/devices/nm-device.c:7499
patterns = <optimized out>
device_driver = 0x564486c76bd0 "veth"
num_patterns = 1
priv = 0x564486cbe0b0
__func__ = "check_connection_compatible"
device_iface = <optimized out>
local = 0x564486c99a60
conn_iface = 0x0
klass = <optimized out>
s_match = 0x564486c63df0 [NMSettingMatch]
#4 0x0000564484c38491 in check_connection_compatible (device=0x564486cbe590 [NMDeviceVeth], connection=0x564486c6b160, error=0x0) at src/core/devices/nm-device-ethernet.c:348
self = 0x564486cbe590 [NMDeviceVeth]
s_wired = <optimized out>
Fixes: 3ced486f41 ('libnm/match: extend syntax for match patterns with '|', '&', '!' and '\\'')
https://bugzilla.redhat.com/show_bug.cgi?id=1942741
