Reported by coverity:
>>> CID 210222: Null pointer dereferences (NULL_RETURNS)
>>> Dereferencing a pointer that might be "NULL" "f" when calling
"fseek".
Fixes: ac5206aa9c ('2007-11-21')
- write_match_setting() never fails. Don't let it return a boolean
error result.
- drop "if (!name || !name[0])" checks. It's not possibly to configure
a name %NULL in NMSettingMatch (without triggering assertions). Also,
an empty name "" is not valid, so we wouldn't expect it. There is one
problem with the way how we concatenate the string list: it uses
spaces as separator, while stripping spaces. That means, in the
currenty format, an empty token "" cannot be expressed. On the other
hand, serializing it would lead to duplicate spaces, that get dropped
during re-read. So the empty name wasn't valid from the start, but it
also cannot be encoded.
- use nm_gstring_add_space_delimiter() and nm_gstring_prepare().
Conceptionally, the MUD URL really depends on the device, and not so
much the connection profile. That is, when you have a specific IoT
device, then this device probably should use the same MUD URL for all
profiles (at least by default).
We already have a mechanism for that: global connection defaults. Use
that. This allows a vendor drop pre-install a file
"/usr/lib/NetworkManager/conf.d/10-mud-url.conf" with
[connection-10-mud-url]
connection.mud-url=https://example.com
Note that we introduce the special "connection.mud-url" value "none", to
indicate not to use a MUD URL (but also not to consult the global connection
default).
The comment isn't right. The fixed array size is in the header file,
because other parts of the code need to know how many elements are in
the array. The alternative would be a define for the size, but that
is only redundant information. Also, even with a define the user who
adds an entry needs to adjust the code in the header. Explicitly stating
the array size in the header makes it almost impossible to accidentally
choosing the wrong size, because the compiler (and unit tests) ensure
the consistency.
Sometimes these function may set errno to unexpected values like EAGAIN.
This causes confusion. Avoid that by using our own wrappers that retry
in that case. For example, in rhbz#1797915 we have failures like:
errno = 0;
v = g_ascii_strtoll ("10", 0, &end);
if (errno != 0)
g_assert_not_reached ();
as g_ascii_strtoll() would return 10, but also set errno to EAGAIN.
Work around that by using wrapper functions that retry. This certainly
should be fixed in glib (or glibc), but the issues are severe enough to
warrant a workaround.
Note that our workarounds are very defensive. We only retry 2 times, if
we get an unexpected errno value. This is in the hope to recover from
a spurious EAGAIN. It won't recover from other errors.
https://bugzilla.redhat.com/show_bug.cgi?id=1797915
Be more graceful and allow whitespaces around the floating point number
for DEVTIMEOUT. Note that _nm_utils_ascii_str_to_int64() is already graceful
against whitespace, so also be it with the g_ascii_strtod() code path.
Scenario:
- have ethernet connection as unmanaged.
- create (or have) a suitable profile for the connection.
- make the device as managed. No default wired connection gets
created.
- delete the profile.
Note that NMManager does in manager_device_state_changed():
»···if (NM_IN_SET (new_state,
»··· NM_DEVICE_STATE_UNAVAILABLE,
»··· NM_DEVICE_STATE_DISCONNECTED))
»···»···nm_settings_device_added (priv->settings, device);
that means, when the device the next time goes through
UNAVAILABLE/DISCONNECTED states, we will suddenly create the
default "Wired connection 1" profile.
That doesn't seem right. When a device is suitable to have a
default-wired connection, we should only check once whether to
create it. We should not retry that later. The !no-auto-default
mechanism exists so we can start NetworkManager without a profile for
the device. It doesn't mean that we later one (after previously deciding
not to create a profile), we still create it.
https://bugzilla.redhat.com/show_bug.cgi?id=1687937https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/450
g_clear_pointer() would always cast the destroy notify function
pointer to GDestroyNotify. That means, it lost some type safety, like
GPtrArray *ptr_arr = ...
g_clear_pointer (&ptr_arr, g_array_unref);
Since glib 2.58 ([1]), g_clear_pointer() is also more type safe. But
this is not used by NetworkManager, because we don't set
GLIB_VERSION_MIN_REQUIRED to 2.58.
[1] f9a9902aac
We have nm_clear_pointer() to avoid this issue for a long time (pre
1.12.0). Possibly we should redefine in our source tree g_clear_pointer()
as nm_clear_pointer(). However, I don't like to patch glib functions
with our own variant. Arguably, we do patch g_clear_error() in
such a manner. But there the point is to make the function inlinable.
Also, nm_clear_pointer() returns a boolean that indicates whether
anything was cleared. That is sometimes useful. I think we should
just consistently use nm_clear_pointer() instead, which does always
the preferable thing.
Replace:
sed 's/\<g_clear_pointer *(\([^;]*\), *\([a-z_A-Z0-9]\+\) *)/nm_clear_pointer (\1, \2)/g' $(git grep -l g_clear_pointer) -i
I think it's preferable to use nm_clear_g_free() instead of
g_clear_pointer(, g_free). The reasons are not very strong,
but I think it is overall preferable to have a shorthand for this
frequently used functionality.
sed 's/\<g_clear_pointer *(\([^;]*\), *\(g_free\) *)/nm_clear_g_free (\1)/g' $(git grep -l g_clear_pointer) -i
Since ifcfg-rh doesn't write out to file the 'connection.timestamp' property
let's add it before comparing an updated connection with the plugin's reread
one otherwise the comparison operation would always fail.
The fix is not necessary for the keyfile plugin, because the reader/writer
correctly reads/writes the connection timestamp.
Don't do a linear search through all names, but use binary search.
Upside: calling nms_ifcfg_rh_utils_get_ethtool_by_name() in a loop
(once over all 60 names) is 75% faster.
Downside: when adding a new feature, we have yet another line that we
need to add. Previously, adding a new feature required adding 7 lines,
not it is 8. But we didn't add a single feature since this was added,
so that happens very seldom.
Possible downside: is this code harder to read? Now we track both how to
convert the ID to name and back. This is redundant (and thus harder to
maintain). But it's really just one extra line per feature, for which there
is a unit test. So, when adding a new NMEthtoolID it would be pretty
hard to mess this up, because of all the tests and assertions.
So, maybe it's slightly harder to read. On the other hand, it unifies
handling for ethtool and kernel names, and the code has less logic
and is more descriptive. I don't think this is actually harder to maintain
and it should be easy to see that it is correct (readability).
Maybe the reader should not try to add its own validation. It
could just read the value, set it in the profile, and let
nm_connection_verify() handle it.
However:
- in this form the code only logs a warning about invalid setting.
If we let it come to nm_connection_verify(), the connection profile
will be entirely rejected. I think this makes sense, because ifcfg
files may be edited by the user and we don't know what is out there.
- it's nicer to show a warning that specifically mentions the DEVICE=
variable. There error message we get from nm_connection_verify()
is no longer aware of ifcfg peculiarities.
Instead: use the appropriate validation function.
Internally, the options are tracked in a hash table and of undefined
sort order. However, nm_setting_bond_get_option() always returns a stable
(sorted) order.
Move "mode" as first, because that is usually the most interesting option.
The effect is:
$ nmcli -o connection show "$BOND_PROFILE"
...
-bond.options: arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,mode=balance-rr,use_carrier=0
+bond.options: mode=balance-rr,arp_interval=5,arp_ip_target=192.168.7.7,arp_validate=active,use_carrier=0
This doesn't affect keyfile, which sorts the hash keys themself (and
doesn't treat the "mode" special).
This however does affect ifcfg-rh writer how it writes the BONDING_OPTS
variable. I think this change is fine and preferable.
nm_utils_is_valid_iface_name() is a public API of libnm-core, let's use
our internal API.
$ sed -i 's/\<nm_utils_is_valid_iface_name\>/nm_utils_ifname_valid_kernel/g' $(git grep -l nm_utils_is_valid_iface_name)
It is undefined behavior and can lead to crashes or memory corruption.
In practice, this only had an issue on Big Endian systems.
Fixes: fdbf4ae5e6 ('ifcfg-rh: add IPV4_DHCP_TIMEOUT key for ipv4.dhcp-timeout property')
If a function is only called once, it may not help to simplify the code
but make it more complicated. It would only simplify the code, if it
had a clear, distinct purpose. That isn't the case here. Also, the
IPv4 writer doesn't have such a function either. Drop and inline it.
