This is severe. We cache the list of names, and we must invalidate the
cache when the names change. Otherwise, out-of-bound access and crash.
Fixes: d0192b698e ('libnm: add nm_setting_option_set(), nm_setting_option_get_boolean(), nm_setting_option_set_boolean()')
Fixes: 150af44e10 ('libnm: add nm_setting_option_get_uint32(), nm_setting_option_set_uint32()')
G_TYPE_STRV is the last property type in NMSetting that is implemented
by directly accessing the GObect property. Note that we have lots of
override, non-default implementations that still use GObject properties,
but I am talking here about properties that don't have a special
implementation and use a G_TYPE_STRV GObject property.
Add a "direct" implementation also for strv arrays.
The advantage is that we no longer call g_value_get() for various
operations, which requires a deep-copy of the strv array. The other
advantage is that we will get a unified approach for implementing strv
properties. In particular strv arrays need a lot of code to implement,
and most settings do it differently. By adding a general mechanism,
this code (and behavior) can be unified.
Showcase it on "match.interface-name".
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.
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.
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.
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.
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
"direct" properties are the latest preferred way to implement GObject
base properties. That way, the property meta data tracks the
"direct_type" and the offset where to find the data in the struct.
That way, we can automatically
- initialize the default values
- free during finalize
- implement get_property()/set_property()
Also, the other settings operations (compare, to/from D-Bus) are
implemented more efficiently and don't need to go through
g_object_get_property()/GValue API.
Certain properties need to release memory when destroying the NMSetting.
For "direct" properties, we have all the information we need to do that
generically in the NMSetting base class. In practice, this only concerns
string properties.
See _finalize_direct() in "nm-setting.c".
However, if the NMSetting base class takes care of freeing the strings,
then the subclasses must not also unref the variable (to avoid double free).
Previously, subclasses had to opt-in for the base class to indicate that
they are fine with that.
Now, let the base class always handle it. We only need to make sure that
classes that implement direct string properties don't also try to free
the values during destruction.
"flags" are a g_param_spec_flags() and correspond to G_TYPE_FLAGS type.
They are internally stored as guint, and exported on D-Bus as "u" (32 bit
integer).
Give a consistent name.
A bit odd are now the names nm_g_bytes_hash() and nm_g_bytes_equal()
as they go together with nm_pg_bytes_hash()/nm_pg_bytes_equal().
But here the problem is more with the naming of "nm_p*_{equal,hash}()"
functions, which probably should be renamed to "nm_*_ptr_{equal,hash}()".
Our handling of properties is relatively complicated. We should have
clear code paths and responsibilities who calls who.
There is from_dbus_fcn() callback to implement parsing a GVariant and
set the property in NMSetting. This is called via:
- _nm_setting_new_from_dbus()
- init_from_dbus()
- _property_set_from_dbus()
Then, one of the from_dbus_fcn() implementations is
_nm_setting_property_from_dbus_fcn_gprop(), which calls
set_property_from_dbus(). That one sets the property using GObject
setter. That's good and a clear code path.
However, set_property_from_dbus() was also called via
- _nm_setting_update_secrets()
- klass->update_one_secret()
- nm-setting.c:update_one_secret()
- set_property_from_dbus()
Meaning, there is a different code path to set_property_from_dbus(),
which bypasses from_dbus_fcn(). That is highly undesirable, because
it should be clear how a property setter gets implemented, and this
way, potentially two different implementations were used.
Refactor nm-setting.c:update_one_secret() to use
_property_set_from_dbus() instead. This behaves potentially differently
for properties like NM_SETTING_ADSL_PASSWORD, which is implemented as
a "direct" property, where from_dbus_fcn() setter no longer uses g_object_set().
This should not make a difference in practice, and in any case, now the
code paths are unified.
Note that most implementations use g_object_set(), and it's not
easy to detect modification. In those cases, we assume that modification
happened -- just like also the GObject setter will emit a notification
(as none of our properties use G_PARAM_EXPLICIT_NOTIFY).
These functions tend to have many arguments. They are also quite som
boilerplate to implement the hundereds of properties we have, while
we want that properties have common behaviors and similarities.
Instead of repeatedly spelling out the function arguments, use a macro.
Advantages:
- the usage of a _NM_SETT_INFO_PROP_*_FCN_ARGS macro signals that this
is an implementation of a property. You can now grep for these macros
to find all implementation. That was previously rather imprecise, you
could only `git grep '\.to_dbus_fcn'` to find the uses, but not the
implementations.
As the goal is to keep properties "similar", there is a desire to
reduce the number of similar implementations and to find them.
- changing the arguments now no longer will require you to go through
all implementations. At least not, if you merely add an argument that
has a reasonable default behavior and does not require explicit
handling by most implementation.
- it's convenient to be able to patch the argument list to let the
compiler help to reason about something. For example, the
"connection_dict" argument to from_dbus_fcn() is usually unused.
If you'd like to find who uses it, rename the parameter, and
review the (few) compiler errors.
- it does save 573 LOC of boilerplate with no actual logic or useful
information. I argue, that this simplifies the code and review, by
increasing the relative amount of actually meaningful code.
Disadvantages:
- the user no longer directly sees the argument list. They would need
cscope/ctags or an IDE to jump to the macro definition and conveniently
see all arguments.
Also use _nm_nil, so that clang-format interprets this as a function
parameter list. Otherwise, it formats the function differently.
There is a quest to move away from the GObject/GValue based setters.
Add _nm_setting_property_from_dbus_fcn_direct(), which can parse
the GVariant and use the direct_type to set the property.
Note that for backward compatibility, we still need
_nm_property_variant_to_gvalue() to convert alternative GVariant
types to the destination value. This means, as before, on the D-Bus
API a property of a certain type can be represented as various D-Bus
types.
This is a normalization employed by NMSettingIPConfig.gateway.
Also rework NMSettingIPConfig.set_property() to no longer assert against
valid input. We want to pass there untrusted strings from D-Bus,
asserting is a horrible idea. Instead, either normalize the string or
keep the invalid text that will be rejected by verify().
A MAC address is a relatively common "type". The GObject property is of type string,
but the D-Bus type is a bytestring ("ay"). We will need a special NMSettInfoPropertType.
Note that like most implementations, the from-dbus implementation still is based
on GObject setters. This will change in the future.
Also note that the previous compare function was
_nm_setting_property_compare_fcn_default(). That is, it used to convert
the property to GVariant and compare those. The conversion to GVariant
in that case normalizes the string (e.g. it is case insensitive). Also,
only properties could be compared which were also convertible to D-Bus
(which is probably fine, because there is no guarantee the profiles that
don't verify can be compared).
The code now uses the direct comparison of the strings. That mostly
preserves the case-insensitivity of the previous comparison, because
the property setters for mac addresses all use
_nm_utils_hwaddr_canonical_or_invalid() to normalize the strings.
This is subtle, but still correct. Note that this will improve later,
by ensuring that the property setters for mac addresses automatically
perform the right normalization.
When looking at a property, it should always be clear how it is handled.
Also the "default" action should be an explicit hook.
Add _nm_setting_property_from_dbus_fcn_gprop() and set that as
from_dbus_fcn() callback to handle the "default" case which us
build around g_object_set_property().
While this adds lines of code, I think it makes the code easier to
understand. Basically, to convert a GVariant to a property, now all
properties call their from_dbus_fcn() handler, there is no special casing.
And the gprop-hook is only called for properties that are using
_nm_setting_property_from_dbus_fcn_gprop(). So, you can reason about
these two functions at separate layers.
NM_SETTING_NAME is also a GObject property, but it's
not supposed to be serialized to/from D-Bus. It also
is irrelevant for comparison.
Hence, it's operations are all NOPs. Make an explicit property type for
that case instead of checking the GParamSpec flags.
The "to_dbus_data" existed for namespacing the properties inside it.
However, such a struct adds overhead due to the alignment that it
enforces. We can share the memory needed for the bitfield by having
them beside each other.
All settings have a "name" property. Their compare_fcn() is not interesting
and was already previously ignored. But we should not special handle it via
_nm_setting_property_compare_fcn_default().
