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).
Coverity warns about this:
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.32.4/src/nmcli/agent.c:87: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.32.4/src/nmcli/agent.c:87: var_assign: Assigning: "pre_input_deftext" = storage returned from "g_strdup(secret->value)".
NetworkManager-1.32.4/src/nmcli/agent.c:87: overwrite_var: Overwriting "pre_input_deftext" in "pre_input_deftext = g_strdup(secret->value)" leaks the storage that "pre_input_deftext" points to.
# 85| /* Prefill the password if we have it. */
# 86| rl_startup_hook = set_deftext;
# 87|-> pre_input_deftext = g_strdup(secret->value);
# 88| }
# 89| if (secret->no_prompt_entry_id)
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.32.4/src/nmcli/common.c:712: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.32.4/src/nmcli/common.c:712: var_assign: Assigning: "nmc_rl_pre_input_deftext" = storage returned from "g_strdup(secret->value)".
NetworkManager-1.32.4/src/nmcli/common.c:712: overwrite_var: Overwriting "nmc_rl_pre_input_deftext" in "nmc_rl_pre_input_deftext = g_strdup(secret->value)" leaks the storage that "nmc_rl_pre_input_deftext" points to.
# 710| /* Prefill the password if we have it. */
# 711| rl_startup_hook = nmc_rl_set_deftext;
# 712|-> nmc_rl_pre_input_deftext = g_strdup(secret->value);
# 713| }
# 714| }
Naming is important, because the name of a thing should give you a good
idea what it does. Also, to find a thing, it needs a good name in the
first place. But naming is also hard.
Historically, some strv helper API was named as nm_utils_strv_*(),
and some API had a leading underscore (as it is internal API).
This was all inconsistent. Do some renaming and try to unify things.
We get rid of the leading underscore if this is just a regular
(internal) helper. But not for example from _nm_strv_find_first(),
because that is the implementation of nm_strv_find_first().
- _nm_utils_strv_cleanup() -> nm_strv_cleanup()
- _nm_utils_strv_cleanup_const() -> nm_strv_cleanup_const()
- _nm_utils_strv_cmp_n() -> _nm_strv_cmp_n()
- _nm_utils_strv_dup() -> _nm_strv_dup()
- _nm_utils_strv_dup_packed() -> _nm_strv_dup_packed()
- _nm_utils_strv_find_first() -> _nm_strv_find_first()
- _nm_utils_strv_sort() -> _nm_strv_sort()
- _nm_utils_strv_to_ptrarray() -> nm_strv_to_ptrarray()
- _nm_utils_strv_to_slist() -> nm_strv_to_gslist()
- nm_utils_strv_cmp_n() -> nm_strv_cmp_n()
- nm_utils_strv_dup() -> nm_strv_dup()
- nm_utils_strv_dup_packed() -> nm_strv_dup_packed()
- nm_utils_strv_dup_shallow_maybe_a() -> nm_strv_dup_shallow_maybe_a()
- nm_utils_strv_equal() -> nm_strv_equal()
- nm_utils_strv_find_binary_search() -> nm_strv_find_binary_search()
- nm_utils_strv_find_first() -> nm_strv_find_first()
- nm_utils_strv_make_deep_copied() -> nm_strv_make_deep_copied()
- nm_utils_strv_make_deep_copied_n() -> nm_strv_make_deep_copied_n()
- nm_utils_strv_make_deep_copied_nonnull() -> nm_strv_make_deep_copied_nonnull()
- nm_utils_strv_sort() -> nm_strv_sort()
Note that no names are swapped and none of the new names existed
previously. That means, all the new names are really new, which
simplifies to find errors due to this larger refactoring. E.g. if
you backport a patch from after this change to an old branch, you'll
get a compiler error and notice that something is missing.
We commonly have strv arrays as (char **), (const char*const*) or
(const char **). We thus need to frequently cast the argument to
nm_utils_strv_find_first().
Explicit casts in C don't make the code more typesafe, because
they silently allow completely wrong casts too. On the other hand,
changing the function argument to (const void *) also allows any
pointer, and not just strv pointers.
NM_CAST_STRV_CC() casts the the pointer to a (const char*const*)
strv pointer. It uses _Generic() to only cast a string array, and
not completely unrelated pointers.
As such, it is more convenient to use, as it requires the user no longer
to cast the strv argument, while still being strict about which types
are accepted.
Dracut supports several options for the "ip=" method.
NetworkManager interprets and handles them in a certain way that aims to
give a similar behavior. But as such it maps different settings ("auth6"
and "dhcp6") to exactly the same behavior.
Add _parse_ip_method() function to normalize these keys, and map their
aliases to the keyword that nm-initrd-generator handles. The advantage
is that you see now in _parse_ip_method() which methods are mapped to
the same behavior, and the later (more complex) code only deals with the
normalized kinds.
Also, use the same validation code at all 3 places where IP methods
can appear, that is
ip=<method>
ip=<ifname>:<method>[:...]
ip=<client-ip>:...:<method>[:...]
Also, dracut supports specifying multiple methods and concatenate them
with comma. nm-initrd-generator only did partly, for example,
`ip=dhcp,dhcp6" would have worked, but only because the code failed
to recognize the string and fell back to the default behavior. It would
not have worked as `ip=<ifname>:dhcp,dhcp6[:...]`. Not all combinations
make sense, but some do. So let _parse_ip_method() detect and handle
them. Currently, they mostly map to "auto", but in the future it might
make sense that `ip=dhcp,local6` is a distinct kind.
Try to tighten up the parsing. It's fine to be forgiving and flexible
about what we parse, but bogus values should not silently be
accepted. However, in order to keep previous behavior, `ip=bogus`
and `ip=<client-ip>:...:<bogus-method>[:...]` explicitly map invalid
method to "auto".
We use NM_PTRARRAY_LEN(), and I find it a bit ugly that a macro does so
much. Maybe, it's better to have it as a function.
But the macro currently lives in "libnm-std-aux/nm-std-aux.h", which
is header-only. To add it to a C source file, we would have to move
it to another header, but "libnm-std-aux/nm-std-aux.h" is nice because
it gets included by default already.
Keep it in "libnm-std-aux/nm-std-aux.h", but implement it as an inline
function.
The macro now only does (as before) some type checking shenanigans to ensure
that the argument is a pointer to pointers.
In practice, there is probably very little difference compared to
the macro before, likely the code will anyway be inlined.
Configuration can have [device*] and [connection*] settings and both
can include a 'match-device=' key, which is a list of device-specs.
Introduce a new 'allowed-connections' key for [device*] sections,
which specifies a list of connection-specs to indicate which
connections can be activated on the device.
With this, it becomes possible to have a device configuration like:
[device-enp1s0]
match-device=interface-name:enp1s0
allowed-connections=except:origin:nm-initrd-generator
so that NM in the real root ignores connections created by the
nm-initrd-generator, and starts activating a persistent
connection. This requires also setting 'keep-configuration=no' to not
generate an assumed connection.
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.
Introduce a user tag key to indicate where the connection comes
from. It would also be possible to have this as a standard property
(as 'connection.origin'), but since this information can be considered
'meta-data' I think the user setting is more appropriate.
Add a new 'keep-configuration' device option, set to 'yes' by
default. When set to 'no', on startup NetworkManager ignores that the
interface is pre-configured and doesn't try to keep its
configuration. Instead, it activates one of the persistent
connections.
To talk to ovsdb, we use the unix socket at
/var/run/openvswitch/db.sock. But that socket is owned by another user
and NetworkManager would need dac_override capability to open it.
We want to drop dac_override, but we still need to talk to ovsdb. Add a
GetFD() method to nm-sudo.
We still first try to open the socket directly. Maybe it just works.
Note that SELinux may block passing file descriptors from nm-sudo. If it
doesn't work for you, test with SELinux permissive mode and wait for an
SELinux update.
NetworkManager runs as root and has lots of capabilities.
We want to reduce the attach surface by dropping capabilities,
but there is a genuine need to do certain things.
For example, we currently require dac_override capability, to open
the unix socket of ovsdb. Most users wouldn't use OVS, so we should
find a way to not require that dac_override capability. The solution
is to have a separate, D-Bus activate service (nm-sudo), which
has the capability to open and provide the file descriptor.
For authentication, we only rely on D-Bus. We watch the name owner
of NetworkManager, and only accept requests from that service. We trust
D-Bus to get it right a request from that name owner is really coming
from NetworkManager. If we couldn't trust that, how could PolicyKit
or any authentication via D-Bus work? For testing, the user can set
NM_SUDO_NO_AUTH_FOR_TESTING=1.
https://bugzilla.redhat.com/show_bug.cgi?id=1921826
NetworkManager (and NetworkManager-all-sym) must not only contain symbols
that are used by itself. Also the device and settings plugin are dlopen'd
by NetworkManager and use symobls form the binary.
That means, if a symbols is only used by a plugin, then we must make sure
that the linker keeps it in the binary. Add a mechanism for that.
g_idle_add() is discouraged, and the checkpatch.pl script warns
about it.
Sometimes there is a legitimate use of it, when you want to always
schedule an idle action (without intent to cancel or track it). That
makes more sense for g_idle_add() than it does for g_timeout_add(),
because a timeout really should be tracked and cancelled if necessary.
Add a wrapper to rename the legitimate uses. This way, we can avoid the
checkpatch.pl warnings, and can grep for the remaining illegitimate uses.
