NMConnection is an interface, implemented by NMSimpleConnection
and NMRemoteConnection. A connection is basically a set of NMSetting
instances.
Usually you would expect that one NMSetting instance only gets added to
zero or one NMConnection. It seems a bit ugly, to have one setting tracked by
multiple NMConnection. Still, technically I am not aware of a single problem
with doing that, where it not for NMSimpleConnection:dispose() to clear the
secrets.
There is no need to clear the secrets of an NMSetting, when the
NMConnection gets destroyed. Either this destroys the NMSetting instance
right away (and NMSetting's destructor will clear the secrets anyway), or
somebody else (e.g. another NMConnection instance), keeps the setting
alive. In the latter case, it is wrong to clear the secrets at
this point.
This was done since commit 6a19e68a7d ('libnm-core: clear secrets from
NMSimpleConnection and NMSettingsConnection dispose()'), but let's stop
doing that.
This also causes problems in practice, see [1].
[1] https://gitlab.gnome.org/GNOME/gnome-control-center/-/merge_requests/1099#note_1334333https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/876https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1056
The n-acd instance gets reset in various cases, for example
when the MAC address changes or during errors.
When that happens, we also need to forget all our pending probes,
because they would reference to a now-defunct n-acd instance.
When the address is currently in state NM_L3_ACD_ADDR_STATE_READY,
we possibly have a pending probe. We need to clean that up. Handle
it the same as in the other cases. Yes, this means we forget about
that the address was ready. But a reset of the n-acd state is a dramatic
event. It warrants some drastic start-over.
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2026288
See-also: https://bugzilla.redhat.com/show_bug.cgi?id=2028422
See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1035
Fixes: 9a76b07f74 ('l3cfg: fix assertion failure')
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.
This partly restores the previous behavior. The point of the
file owner check is to ensure that the file cannot be read
by unpriviledged processes as it may contain secrets. If the
file is owned by root, that is considered secure (even if our
euid is different).
Possibly, if our euid is not root, then we couldn't read the
file, but that is a different problem.
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.
All callers of _nm_setting_get_private() got the offset from the
property info. Add a wrapper _nm_setting_get_private_field() that
takes the property info. This way, it can add some assertions.
Preferably, we embed the private struct in the GObject struct itself.
In the past, we didn't do that, because the struct was in public headers
and changing that would have been an ABI break. For those struct, we
still use g_type_class_add_private().
We have some structs, where the private struct is embedded. An
alternative to that would be, to not have the private struct at all,
like done for NMSettingOvsBridge.
Anyway. So for direct properties we need to capture the offset of the
field (in the private struct). We can either set the offset of the
private struct in _nm_setting_class_commit() to zero and let the field
offset include the private structure offset. Or, the offset of the
private struct is accounted during _nm_setting_class_commit().
Both approaches are basically the same. Just do it consistently. For no
particular reason, choose to set the offset of the private data to zero
for those types.
Several properties like "connection.type" are enum-like and only take a few
known values. We can use a NMRefString to share their instances.
Currently nm_setting_duplicate() does not yet explicitly handle direct properties.
But it should, because it can handle them more efficiently. If it would do that, it
would be very cheap to "copy" a NMRefString. But even with the current implementation
will the result be deduplicated.
"wireguard.private-key" is special, because the setter does some unusual
normalization. To implement that, we need to use "direct_hook.set_string_func".
We want that our properties have little special cases and follow a
few common behaviors. For example, we have string properties, and those
should mostly behave the same (e.g. by being "direct-string"
properties).
That is already not fully enough, because we have slightly different
behaviors. For example, we have string properties that should have their
whitespace stripped, that should be ascii case down converted, that
should be normalized IP or MAC addresses. So far, that was expressed via
simple fields in NMSettInfoProperty, like NMSettInfoProperty's
direct_set_string_ascii_strdown field.
But that is not enough. In particular, for "wireguard.private-key" we
perform a different kind of normalization (base64 parsing, and taking
care not to leak secret in memory). It seems to special to add a boolean
flag "direct_set_string_wireguard_private_key".
Instead, add a hook that can cover that.
We need a hook, because we want one setter implementation throughout. Commonly,
we have at least two setters: the GObject set_property() and from D-Bus.
Both should call into the same underlying implementation, to avoid code
duplication. For that, the tweaked behavior must be "down", that is at
the deepest point in the call stack where we set the string. That's why
we need the hook. The alternative would be two special implementation
for GObject and D-Bus setters (and in the future we might add setters
from keyfile).
Both callers themselves needed to call _nm_setting_get_private(),
only to pass it to _property_direct_set_string().
Instead, pass the necessary parameters to _property_direct_set_string(),
so it can do that itself.
This additional parameters will be necessary when we add a hook for
setting the string.
We cache the virtual-iface-name. The caching is also part of the API as
nm_setting_infiniband_get_virtual_interface_name() returns a const
string.
As the value is computed and based on the parent and the p-key, we must
clear the cache when the parent or p-key changes (or detect that it's
invalid).
Previously, we were simply clearing the value in the set_property() function,
which is the only setter of these two properties. If we make these
properties "direct properties", then they will be directly set via
from_dbus_fcn() which bypasses the GObject setter. Which is a problem
for the cache invalidation.
We could either not make those properties direct properties. The problem
is that direct properties are nice, and they will in the future
implement further optimizations for them. Also, they are the default
implementation, and it seems clearer to build something on top of that,
instead of deviating from the default.
Instead, let the caching detect when the value needs to be regenerated.
This seems a questionable thing to do, and should be made clearer by
having a parameter (that makes you think about what is happening here).
Also, the normalization for vxlan.remote does not perform this mapping,
so the parameter is there so that the approach can handle both flavors.
Let's sprinkle some snake ointment.
This is questionable, because we copy secrets all over the place where
we their deallocation (and clearing) is not in our control. For example,
the GValue setter/getter copies the string (but does not clean the
secret). Also, when converting the property to a GVariant, we won't
clear it. So this does not catch a lot of cases.
Still, if we can with relative ease avoid leaking the string at some
places, do it.
libnm's data structures are commonly not thread safe (like
NMConnection). However, it must be possible that all operations can
operate on *different* data in a thread safe manner. That means, we need
to take care about our global variables.
nm_utils_ssid_to_utf8() uses a list of encodings, which gets cached.
- replace the GHashTables with a static list. Since it doesn't cost
anything, make the list sorted and look it up via binary search.
nm_hash_str() has the proper name and signature for that it does.
That is, it has a "nm_hash_*" prefix and the parameter is of type
"const char *".
nm_hash_str() has this name because it is primarily about hashing.
For hash tables, glib has g_str_hash() and g_str_equal(). We want
to replace g_str_hash() with our implementation (nm_hash_str()) because
that uses siphash24 with a random seed.
But in those cases we want to use the more familiar name "nm_str_hash()",
which reminds of g_str_hash() and follows the pattern of g_str_equal().
Thus:
g_hash_table_new(nm_str_hash, g_str_equal);
is preferable over
g_hash_table_new(nm_hash_str, g_str_equal);
Hence, we have (and had) nm_str_hash() effectively an alias to
nm_hash_str.
The question is: which name is preferable? Or should they be both present
for their slightly distinct uses? The approach taken here is to have
both names, to reflect their purpose better.
But as the usage of nm_str_hash is as function pointer for GHashTable, it was
not an inline function and we'd pay a small overhead with this approach of
aliasing. Avoid that overhead by defining nm_str_hash with the C
preprocessor.
For similar reasons, do that for nm_direct_hash.
