NetworkManager is single-threaded and uses a mainloop.
However, sometimes we may need multiple threads. For example, we will
need to write sysctl values asynchronously, using the glib thread-pool.
For that to work, we also need to switch the network-namespace of the
thread-pool thread. We want to use NMPNetns for that. Hence it's better
to have NMPNetns thread-safe, instead of coming up with a duplicate
implementation. But NMPNetns may want to log, so we also need nm-logging
thread-safe.
In general, code under "shared/nm-utils" and nm-logging should be usable
from multiple threads. It's simpler to make this code thread-safe than
re-implementing it. Also, it's a bad limitation to be unable to log
from other threads. If there is an error, the best we can often do is to
log about it.
Make nm-logging thread-safe. Actually, we only need to be able to log
from multiple threads. We don't need to setup or configure logging from
multiple threads. This restriction allows us to access logging from the
main-thread without any thread-synchronization (because all changes in
the logging setup are also done from the main-thread).
So, while logging from other threads requires a mutex, logging from the
main-thread is lock-free.
There is:
1) glib's MAX() macro, which evaluates arguments multiple times,
but yields a constant expression, if the arguments are constant.
2) NM's NM_MAX() macro, which evaluates arguments exactly once,
but never yields a constant expression.
3) systemd's MAX() which is like NM_MAX().
Now, it's sensible to use
char buf[MAX (A_CONSTANT, ANOTHER_CONSTANT)];
and this works with glib's variant (1).
However, when we include systemd headers, 1) gets redefined to 3), and
above no longer works. That is because we we don't allow VLA and systemd's
macro gives not a constant expression.
Add NM_CONST_MAX() macro which is like systemd's CONST_MAX(). It can
only operate on constant arguments.
We named the types inconsistently:
- "p2p-wireless" ("libnm-core/nm-setting-p2p-wireless.h")
- "p2p" ("libnm/nm-p2p-peer.h")
- "p2p-wifi" ("src/devices/wifi/nm-device-p2p-wifi.h")
It seems to me, "libnm/nm-p2p-peer.h" should be qualified with a "Wi-Fi"
specific name. It's not just peer-to-peer, it's Wi-Fi P2P.
Yes, there is an inconsistency now, because there is already
"libnm/nm-access-point.h".
It seems to me (from looking at the internet), that the name "Wi-Fi P2P"
is more common than "P2P Wi-Fi" -- although both are used. There is also
the name "Wi-Fi Direct". But it's not clear which name should be
preferred here, so stick to "Wi-Fi P2P".
In this first commit only rename the files. The following commit will
rename the content.
NMIPAddr is a union of IPv4 and IPv6 addresses.
A lot of our internal API handles IPv4 as in_addr_t / guint32 / be32_t
types, as such the union field "addr4" is just a plain number. Possibly
the internal API should be all refactored to prefer "struct in_addr"
instead, but that is yet to be done.
Anyway, at a few places we will need also access to the IPv4 address in form of
a `struct in_addr`. Add an alias for that.
I am not too happy about the resulting naming. It would be nicer to have
struct in_addr addr4;
struct in6_addr addr6;
in_addr_t s_addr4;
but for now, don't do such renaming.
NM_UTILS_LOOKUP_STR() uses alloca(). Partly to avoid the overhead of
malloc(), but more important because it's convenient to use. It does
not require to declare a varible to manage the lifetime of the heap
allocation.
It's quite safe, because the stack allocation is of a fixed size of only
a few bytes. Overall, I think the convenience that we get (resulting in
simpler code) outweighs the danger of stack allocation in this case. It's
still worth it.
However, as it uses alloca(), it still must not be used inside a (unbound)
loop and it is obviously a macro.
Rename the macros to have a _A() suffix. This should make the
peculiarities more apparent.
Add a version of nm_utils_strbuf_append_*() that does not care
about NUL terminate strings, but accept any binary data. That makes
it useful for writing a binary buffer.
Since we already cached the result of getpagesize() in a static variable (at
two places), move the code to nm-shared-utils, so it is reusable.
Also, use sysconf() instead of getpagesize(), like suggested by `man
getpagesize`.
Using strncpy() in the macro directly can result in a compiler warning.
We don't want to replace this with memcpy(), because strncpy() aborts
on the first NUL and fills the rest with NUL. Since nm_strndup_a() is a
replacement for g_strndup(), we want to do that here as well.
In file included from ../shared/nm-default.h:294,
from ../libnm-core/nm-utils.c:22:
../libnm-core/nm-utils.c: In function nm_sock_addr_endpoint_new:
../shared/nm-utils/nm-shared-utils.h:281:4: error: strncpy output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
strncpy (_s, _str, _len); \
^~~~~~~~~~~~~~~~~~~~~~~~
../libnm-core/nm-utils.c:154:26: note: in expansion of macro nm_strndup_a
host = _parse_endpoint (nm_strndup_a (200, endpoint, l_endpoint - 1, &host_clone),
^~~~~~~~~~~~
../libnm-core/nm-utils.c:152:15: note: length computed here
l_endpoint = strlen (endpoint) + 1;
^~~~~~~~~~~~~~~~~
Add helper wrappers around nm_g_object_set_property() that take a
native value, construct a GValue of the according type, and call
nm_g_object_set_property().
Commonly, the prefix is a string constant. We don't need to call
g_str_has_prefix() for that, which first requires strlen() on
the prefix. All the information is readily available.
Add a macro for that.
It's not yet used, but it will be. We will need nm_sd_utils_unbase64mem()
to strictly validate WireGuard settings, which contain keys in base64 encoding.
Note that we also need a stub implementation for logging. This will do
nothing for all logging from "libnm-systemd-shared.a". This makes
sense because "libnm.so" as a library should not log directly. Also,
"libnm.so" will only use a small portion of "libnm-systemd-shared.a" which
doesn't log anything. Thus this code is unused and dropped by the linker
with "--gc-sections".
glib has an base64 implementation, but g_base64_decode() et al. gives
no way to detect invalid encodings. All invalid codes are silently
ignored. That is not suitable for strictly validating user input.
Instead of reimplementing of copy-pasting the code from somewhere,
reuse systemd's unbase64mem().
But don't use "hexdecoct.h" directly. Instead, add a single accessor
function to our "nm-sd-utils-shared.h" gateway. We want to be careful
about which bits from systemd we use, because otherwise re-importing
systemd code becomes fragile as you don't know which relevant parts
changed.
For better or worse, we already pull in large parts of systemd sources.
I need a base64 decode implementation (because glib's g_base64_decode()
cannot reject invalid encodings). Instead of coming up with my own or
copy-paste if from somewhere, reuse systemd's unbase64mem().
But for that, make systemd's basic bits an independent static library
first because I will need it in libnm-core.
This doesn't really change anything except making "libnm-systemd-core.la"
an indpendent static library that could be used from "libnm-core". We
shall still be mindful about which internal code of systemd we use, and only
access functionality that is exposed via "systemd/nm-sd-utils-shared.h".
C11 provides _Generic(). Until now we used it when the compiler supports
it (in extended --std=gnu99 mode). In practice, now that we require C11
it should always be present.
We will drop compatibility code in the future. For now, just add a comment
and keep it. The reason is, that "shared/nm-utils/nm-macros-internal.h"
may be used by VPN plugins or applet, which may or may not yet bump to
C11. Keeping it for now, allows for an easier update.
In core ("src/"), we use "nm-logging.h" for all logging. This dispatches
for logging to syslog, glog or systemd-journald.
If we want to log from a shared component under "shared/", we need to
use a common logging function. Add "nm-utils/nm-logging-fwd.h" for
forward declaring the used logging mechaism.
The shared library will still need to link with "src/nm-logging.c"
or an alternative implementation, depending on whether it is used
inside core or not.
- in nm_keyfile_read(), unify _read_setting() and
_read_setting_vpn_secret() in they way they are called
(that is, they no longer return any value and don't accept
any arguments aside @info).
- use cleanup attributes
- use nm_streq() instead of strcmp().
- wrap lines that have multiple statements or conditions.
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from <errno.h>, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from <errno.h> or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.
This is also what systemd does, except that systemd only returns
(negative) integers from <errno.h>, while we merge our own error codes
into the range of <errno.h>.
The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from <errno.h>, but it did not entail error codes from netlink.
This will be our extension on top of <errno.h>.
We want to use (integer) error numbers, that can both
contain native errors from <errno.h> and our own defines,
both merge in one domain. That is, we will reserve a small
range of integers for our own defines (that hopefully won't
clash with errors from <errno.h>).
We can use this at places where GError is too cumbersome to use.
The advantage is, that our error numbers extend <errno.h> and can
be mixed.
This is what "src/platform/nm-netlink.h" already does with nl_errno(). Next,
the netlink errors from there will be merged into "nm-errno.h".
Also, platform has NMPlatformError, which are a distinct set of error
numbers. But these work differently in the sense that negative values
represent codes from <errno.h> and positive numbers are our own platform
specific defines. NMPlatformError will also be merged into "nm-errno.h".
"nm-errno.h" will unify the error handling of platform and netlink,
making it more similar to what we are used to from systemd, and give
room to extend it for our own purpose.
Preface: RFC 3442 (The Classless Static Route Option for Dynamic Host
Configuration Protocol (DHCP) version 4) states:
If the DHCP server returns both a Classless Static Routes option and
a Router option, the DHCP client MUST ignore the Router option.
Similarly, if the DHCP server returns both a Classless Static Routes
option and a Static Routes option, the DHCP client MUST ignore the
Static Routes option.
Changes:
- sd_dhcp_lease_get_routes() returns the combination of both option 33
(static routes) and 121 (classless static routes). If classless static
routes are provided, the state routes must be ignored.
- we collect the options hash that we expose on D-Bus. For that purpose,
we must not merge both option types as classless static routes. Instead,
we want to expose the values like we received them originally: as two
different options.
- we continue our deviation from RFC 3442, when receiving classless static
routes with option 3 (Router), we only ignore the router if we didn't
already receive a default route via classless static routes.
- in the past, NetworkManager treated the default route specially, and
one device could only have one default route. That limitation was
already (partly) lifted by commit 5c299454b4
(core: rework tracking of gateway/default-route in ip-config). However,
from DHCP we still would only accept one default route. Fix that for
internal client. Installing multiple default routes might make sense, as
kernel apparently can skip unreachable routers (as it notes via ICMP
messages) (rh#1634657).
https://bugzilla.redhat.com/show_bug.cgi?id=1634657
We have nm_utils_inet*_ntop(), however:
- that is partly private API libnm-core, and thus only available in
components that have access to that. Partly it's public API of
libnm, but still only available in components that use libnm.
- relying on the static buffers is discouraged for nm_utils_inet*_ntop().
For testing, that is fine as we are in a more controlled envionment.
So, add a test variant that explicitly relies on static buffers.
That way, it's more convenient to use from tests.
- these functions can assert more and are more convenient to use from
tests.
With the default 128KiB buffer size it is easy to lose events. For
example when 64 interfaces appear at the same time, we lose events for
the last 16. Increase the buffer size to 4MiB.
https://bugzilla.redhat.com/show_bug.cgi?id=1651578
CSiphash is a first class citizen, it's fine to use everwhere where we
need it.
NMHash wraps CSiphash and provides three things:
1) Convenience macros that make hashing nicer to use.
2) it uses a randomly generated, per-run hash seed, that can be combined
with a guint static seed.
3) it's a general API for hashing data. It nowhere promises that it
actually uses siphash24, although currently it does everywhere.
NMHash is not (officially) siphash24.
Add API nm_hash_siphash42_init() and nm_hash_siphash42() to "nm-hash-utils.h",
that exposes (2) for use with regular CSiphash. You of course no longer
get the convenice macros (1) but you get plain siphash24 (which
NMHash does not give (3)).
While at it, also add a nm_hash_complete_u64(). Usually, for hasing we
want guint types. But we don't need to hide the fact, that the
underlying value is first uint64. Expose it.
These are simple macros/functions that append a heap-allocated string to
a GPtrArray. It is intended for a GPtrArray which takes ownership of the
strings (meaning, it has a g_free() GDestroyNotify).
From [1]:
You may optionally specify attribute names with ‘__’ preceding and
following the name. This allows you to use them in header files
without being concerned about a possible macro of the same name. For
example, you may use the attribute name __noreturn__ instead of
noreturn.
[1] https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax
Add a new CON_DEFAULT() macro that places a property name into a
special section used at runtime to check whether it is a supported
connection default.
Unfortunately, this mechanism doesn't work for plugins so we have to
enumerate the connection defaults from plugins in the daemon using
another CON_DEFAULT_NOP() macro.
Supporting a trailing comma in NM_MAKE_STRV() can be desirable, because it
allows to extend the code with less noise in the diff.
Now, there may or may not be a trailing comma at the end.
There is a downside to this: the following no longer work:
const char *const v1[] = NM_MAKE_STRV ("a", "b");
const char *const v2[3] = NM_MAKE_STRV ("a", "b");
but then, above can be written more simply already as:
const char *const v1[] = { "a", "b", NULL };
const char *const v2[3] = { "a", "b" };
so the fact that the macro won't work in that case may be preferable,
because it forces you to use the already existing better variant.
nm_utils_get_monotonic_timestamp*() inherrently use static data. Let's
initialize it in a thread safe manner.
nm_utils_get_monotonic_timestamp*() are a fundamental utility function
that should work correctly in all cases. Such a low level function should
be thread safe.
The GChecksum API is cumbersome to use.
For example, g_checksum_get_digest() requires a length input/output
argument. At the same time, GChecksum does not allow you to query its
checksum-type nor the desired digest-length. When you have a GChecksum
at hand, you must always know the digest-length you are going to use.
So, the length parameter is only good for asserting.
Add a macro to make that more convenient.
Benefits: it's less lines of code, and we always do all the asserts
that are due.
- fix thread-safety by adding a memory barrier (g_atomic_pointer_get())
to the double-checked locking pattern when initializing the hash key.
- generate the random data outside the lock. Calling nm_utils_random_bytes()
within the lock is ugly, because we don't want to assume that the function
has no side effects which are prone to dead-lock. There is no problem attempting
to generate the random data without lock, and only use it when the race is won.
Often, during tests we want to assert against the logged messages.
In fact, most tests enable assertions for all logging and enforce
them with g_test_assert_expected_messages(). So, this is common.
However, sometimes it can be cumbersome to understand which logging
lines will be produced. For example, the next commits will call
nm_dhcp_manager_get() during the tests, which initializes NMDhcpManager
and logs a message which plugin was selected (or an additional warning,
if the selected plugin was not found). The availability of the DHCP plugin
depends on searching the path for "/usr/bin/dhclient", so from testing code
it's hard to determine what will be logged.
Instead, add a way to temporarily disable logging during testing.
If passed a relative path, load the editor .so from the same directory
as the plugin .so. This is useful for development, as it allows running
the editor plugin from the build tree conveniently.
https://github.com/NetworkManager/NetworkManager/pull/242
Refactor the check so that integer overflow cannot happen. Realistically,
it anyway couldn't happen, because _name is nowhere near the size of
G_MAXSIZE. Still, avoid such code. Also, the operands involved here are
constants, so the extra check can anyway be resolved at compile-time.
"shared/nm-utils" is a loose collection of utility functions.
There is a certain aim that they can be used independently.
However, they also rely on each other.
Add a test that we can build a minimal shared library with
these tools, independent of libnm-core.
This is independent functionality that only depends on linux API
and glib.
Note how "nm-logging" uses this for getting the timestamps. This
makes "nm-logging.c" itself dependen on "src/nm-core-utils.c",
for little reason.
1. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1242: value_overwrite: Overwriting previous write to "ch" with value from "v".
2. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1239: assigned_value: Assigning value from "++str[0]" to "ch" here, but that stored value is overwritten before it can be used.
# 1237| if (ch >= '0' && ch <= '7') {
# 1238| v = v * 8 + (ch - '0');
# 1239|-> ch = (++str)[0];
# 1240| }
# 1241| }
Don't assign ch when it is going to be overwritten.
keyfile already supports omitting the "connection.id" and
"connection.uuid". In that case, the ID would be taken from the
keyfile's name, and the UUID was generated by md5 hashing the
full filename.
No longer do this during nm_keyfile_read(), instead let all
callers call nm_keyfile_read_ensure_*() to their liking. This is done
for two reasons:
- a minor reason is, that one day we want to expose keyfile API
as public API. That means, we also want to read keyfiles from
stdin, where there is no filename available. The implementation
which parses stdio needs to define their own way of auto-generating
ID and UUID. Note how nm_keyfile_read()'s API no longer takes a
filename as argument, which would be awkward for the stdin case.
- Currently, we only support one keyfile directory, which (configurably)
is "/etc/NetworkManager/system-connections".
In the future, we want to support multiple keyfile dirctories, like
"/var/run/NetworkManager/profiles" or "/usr/lib/NetworkManager/profiles".
Here we want that a file "foo" (which does not specify a UUID) gets the
same UUID regardless of the directory it is in. That seems better, because
then the UUID won't change as you move the file between directories.
Yes, that means, that the same UUID will be provided by multiple
files, but NetworkManager must already cope with that situation anyway.
Unfortunately, the UUID generation scheme hashes the full path. That
means, we must hash the path name of the file "foo" inside the
original "system-connections" directory.
Refactor the code so that it accounds for a difference between the
filename of the keyfile, and the profile_dir used for generating
the UUID.
In general, it's fine to pass %NULL to g_free().
However, consider:
char *
foo (void)
{
gs_free char *value = NULL;
value = g_strdup ("hi");
return g_steal_pointer (&value);
}
gs_free, gs_local_free(), and g_steal_pointer() are all inlinable.
Here the compiler can easily recognize that we always pass %NULL to
g_free(). But with the previous implementation, the compiler would
not omit the call to g_free().
Similar patterns happen all over the place:
gboolean
baz (void)
{
gs_free char *value = NULL;
if (!some_check ())
return FALSE;
value = get_value ();
if (!value)
return FALSE;
return TRUE;
}
in this example, g_free() is only required after setting @value to
non-NULL.
Note that this does increase the binary side a bit (4k for libnm, 8k
for NetworkManager, with "-O2").
Add a configure option to disable eBPF support in n-acd.
Note that, even if eBPF is not supported, n-acd requires a kernel >
3.19, which means that the setsockopt(..., SO_ATTACH_BPF) option must
be defined. To allow building on older kernels without modifying the
n-acd code, we inject the SO_ATTACH_BPF value as a preprocessor define
in the compiler the command line.
The assertion fails in nmtui's ip_route_transform_from_dest_string(),
which does not initialize the address output argument to %NULL.
There are three possibilities how the API could work:
- assert/require the user to pass in arguments which pre-initialized
to NULL or unset.
- always set the output arguments, even if the function fails.
- don't bother and leave output values untouched, if function fails.
It's not clear which approach is the best. Not to bother possibliy
leaves uninitialized values, which could be error prone. Still, do
just that.
Fixes: 0b3197a3fd
As we accept addr_family %AF_UNSPEC to detect the address family,
we also need to return it. Just returning the binary address without
the address family makes no sense.
Ok, I changed my mind.
The new behavior seems to make more sense to me. Not that it matters,
because we always use nm_utils_strbuf*() API with buffers that we expect
to be large enough to contain the result. And when truncation occurs,
we usually don't care much about it. That is, there is no code that
uses nm_utils_strbuf*() API and handles string truncation in particular.
- previously, parsing wireguard genl data resulted in memory corruption:
- _wireguard_update_from_allowedips_nla() takes pointers to
allowedip = &g_array_index (buf->allowedips, NMWireGuardAllowedIP, buf->allowedips->len - 1);
but resizing the GArray will invalidate this pointer. This happens
when there are multiple allowed-ips to parse.
- there was some confusion who owned the allowedips pointers.
_wireguard_peers_cpy() and _vt_cmd_obj_dispose_lnk_wireguard()
assumed each peer owned their own chunk, but _wireguard_get_link_properties()
would not duplicate the memory properly.
- rework memory handling for allowed_ips. Now, the NMPObjectLnkWireGuard
keeps a pointer _allowed_ips_buf. This buffer contains the instances for
all peers.
The parsing of the netlink message is the complicated part, because
we don't know upfront how many peers/allowed-ips we receive. During
construction, the tracking of peers/allowed-ips is complicated,
via a CList/GArray. At the end of that, we prettify the data
representation and put everything into two buffers. That is more
efficient and simpler for user afterwards. This moves complexity
to the way how the object is created, vs. how it is used later.
- ensure that we nm_explicit_bzero() private-key and preshared-key. However,
that only works to a certain point, because our netlink library does not
ensure that no data is leaked.
- don't use a "struct sockaddr" union for the peer's endpoint. Instead,
use a combintation of endpoint_family, endpoint_port, and
endpoint_addr.
- a lot of refactoring.
I think g_memdup() is dangerous for integer overflow. There
is no need for accepting this danger, just use our own nm_memdup()
which does not have this flaw.
PROP_0 is how we commonly name this property when we don't use
NM_GOBJECT_PROPERTIES_DEFINE(). Rename it.
Also, allow to skip PROP_0 in nm_gobject_notify_together(), that
is handy to optionally invoke a notification, like
nm_gobject_notify_together (obj,
PROP_SOMETHING,
changed ? PROP_OTHER : PROP_0);
