However, don't also use the NM_DEPRECATED_IN_1_32 macro, because that
causes annoying compiler warnings.
There is no replacement for the function in libnm, nor is it planned
to add one. So users may still call it, but they are now warned by
documentation that it may not be a good idea.
In the past, the UUID was only loosely validate and would accept
forms that are not valid. This was fixed by commit 207cf3d5d4 ('libnm:
normalize "connection.uuid"'). Now the UUID is always strictly valid
and lower case.
Thus, don't use the fuzzy nm_utils_is_uuid() from libnm but the exact
check nm_uuid_is_valid_full().
Note that this is only used for assertions in the header file. We thus
don't want to drag in "libnm-glib-aux/nm-uuid.h". Instead, we forward
declare the function.
lgtm.com warns about declarations are block scope, so fix that too by
moving the declaration at file scope.
lgtm.com warns about function declarations inside blocks.
*sigh*. I think it's well understood what this code means, and it is not
done by accident. Still, let's make the tool happy in this case.
lgtm.com warns about these uses. They are correct though. Maybe the code should
not use alloca() simply to suppress the warning. Instead, add a comment pointing
out that this is in fact correct.
For one, "src_n_map" must always be greater than zero at this point.
lgtm.com warns about that, and the point of this patch is to avoid
that warning.
Still, the check really isn't needed, also because nm_memdup() explicitly
handles buffers sizes of zero.
Since the [main].iwd-config-path functionality, where NM watches for
NMSettingsConnection changes and update IWD network config files with
new settings, has proven to work without issues so far, enable it by
default. Instead of hardcoding /var/lib/iwd as the value, and since the
value can't be probed at NM compile time, query it from IWD's recently-
added D-Bus interface for settings when [main].iwd-config-path is either
missing or set to the new value "auto".
Coverity has issues with functions that handle ownership like
g_strstrip(). Thus the scan is full of false positives like:
Error: RESOURCE_LEAK (CWE-772): [#def45] [important]
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: noescape: Resource "g_strdup(attribute_values[i])" is not freed or pointed-to in "g_strchug".
NetworkManager-1.31.5/src/core/devices/wwan/nm-service-providers.c:134: leaked_storage: Failing to save or free storage allocated by "g_strdup(attribute_values[i])" leaks it.
# 132| if (strcmp(attribute_names[i], "value") == 0) {
# 133| parse_context->state = PARSER_METHOD_GSM_APN;
# 134|-> parse_context->apn = g_strstrip(g_strdup(attribute_values[i]));
# 135| break;
# 136| }
Add a workaround for that.
There are other functions that have the same problem, but the usage
g_strstrip(g_strdup(...)) is common to warrant a special workaround.
It's a false positive. Still avoid it.
Error: FORWARD_NULL (CWE-476): [#def479]
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: var_deref_model: Passing "strbuf" to "nm_str_buf_maybe_expand", which dereferences null "strbuf->_priv_str".
# 5573| l2 = ((gsize) l) + 1u;
# 5574|
# 5575|-> nm_str_buf_maybe_expand(strbuf, l2, FALSE);
# 5576|
# 5577| va_start(args, format);
Error: FORWARD_NULL (CWE-476): [#def480]
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5558: var_compare_op: Comparing "strbuf->_priv_str" to null implies that "strbuf->_priv_str" might be null.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5575: no_write_call: Although "nm_str_buf_maybe_expand" does overwrite "strbuf->_priv_str" on some paths, it also contains at least one feasible path which does not overwrite it.
NetworkManager-1.31.5/src/libnm-glib-aux/nm-shared-utils.c:5578: var_deref_op: Dereferencing null pointer "strbuf->_priv_str".
# 5576|
# 5577| va_start(args, format);
# 5578|-> l = g_vsnprintf(&strbuf->_priv_str[strbuf->_priv_len], l2, format, args);
# 5579| va_end(args);
# 5580|
Also Coverity found that something is wrong here:
Error: FORWARD_NULL (CWE-476): [#def361]
NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:441: var_compare_op: Comparing "connection" to null implies that "connection" might be null.
NetworkManager-1.31.5/src/libnm-client-impl/nm-vpn-plugin-old.c:489: var_deref_model: Passing null pointer "connection" to "g_object_unref", which dereferences it.
# 487| }
# 488|
# 489|-> g_object_unref(connection);
# 490| }
# 491|
Fixes: 6793a32a8c ('libnm: port to GDBus')
Previously, we would allocate a buffer of the worst case, that is,
4 times the number of bytes, in case all of them require octal escaping.
Coverity doesn't like _escape_ansic() for another reason:
Error: NULL_RETURNS (CWE-476): [#def298]
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: returned_null: "g_malloc" returns "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:161: alias: Assigning: "q" = "dest = g_malloc(strlen(source) * 4UL + 1UL + 3UL)". Both pointers are now "NULL".
NetworkManager-1.31.5/src/core/settings/plugins/ifcfg-rh/shvar.c:163: dereference: Incrementing a pointer which might be null: "q".
# 161| q = dest = g_malloc(strlen(source) * 4 + 1 + 3);
# 162|
# 163|-> *q++ = '$';
# 164| *q++ = '\'';
# 165|
It doesn't recognize that g_malloc() shouldn't return NULL (because
we never request zero bytes).
I am not sure how to avoid that, but let's rework the code to first count
how many characters we exactly need. It think that should also help with
the coverity warning.
Doing exact allocation requires first to count the number of required
bytes. It still might be worth it, because we might keep the allocated
strings a bit longer around.
It seems very ugly to read one byte at a time. Use a naive buffered
reader, so that we can read multiple bytes at a time, and return them
one by one.
Also, this now keeps state of any error/EOF. Once we reach EOF, we won't
read again. The previous code did that too, but I think this code is
easier to read.
g_warning() and printing to stdout/stderr are not suitable actions
for a library. If there is something important, find a way to report the
condition to the caller. If it's not important, stay quiet.
"val" and "key" are now marked as nm_auto. They are freed at the end,
and we should not free them before breaking the loop (at least not,
without also clearing the variables).
Fixes: 02dbba49d6 ('libnm: fix leak in nm_vpn_service_plugin_read_vpn_details()')
Since g_string_free() takes an additional argument,
it's not direclty usable with nm_clear_pointer(ptr, g_string_free);
As workaround, add nm_clear_g_string() helper.
Coverity doesn't like the previous code:
Error: RESOURCE_LEAK (CWE-772): [#def34] [important]
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: alloc_fn: Storage is returned from allocation function "g_strdup".
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: noescape: Resource "g_strdup(config)" is not freed or pointed-to in "g_strdelimit".
NetworkManager-1.31.5/src/core/devices/team/nm-device-team.c:835: leaked_storage: Failing to save or free storage allocated by "g_strdup(config)" leaks it.
# 833| char *sanitized_config;
# 834|
# 835|-> sanitized_config = g_strdelimit(g_strdup(config), "\r\n", ' ');
# 836| err = teamdctl_port_config_update_raw(priv->tdc, slave_iface, sanitized_config);
# 837| g_free(sanitized_config);
Maybe this works better.
When the ACCEPT_ALL_MAC_ADDRESSES key is found by the wired reader, the
wired setting was not being created.
Fixes: d946aa0c50 ('wired-setting: add support to accept-all-mac-addresses')
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Commit 33b9fa3a3c ("manager: Keep volatile/external connections
while referenced by async_op_lst") changed active_connection_find() to
also return active connections that are not yet activating but are
waiting authorization.
This has side effect for other callers of the function. In particular,
_get_activatable_connections_filter() should exclude only ACs that are
really active, not those waiting for authorization.
Otherwise, in ensure_master_active_connection() all the ACs waiting
authorization are missed and we might fail to find the right master
AC.
Add an argument to active_connection_find to select whether include
ACs waiting authorization.
Fixes: 33b9fa3a3c ('manager: Keep volatile/external connections while referenced by async_op_lst')
https://bugzilla.redhat.com/show_bug.cgi?id=1955101
A user might configure /etc/dhcpcd.conf to contain static fallback addresses.
In that case, the dhcpcd plugin reports the state as "static". Let's treat
that the same way as bound.
Note that this is not an officially supported or endorsed way of
configuring fallback addresses in NetworkManager. Rather, when using
DHCP plugins, the user can hack the system and make unsupported
modifications in /etc/dhcpcd.conf or /etc/dhcp. This change only makes
it a bit easier to do it.
See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/579#note_922758https://bugzilla.gnome.org/show_bug.cgi?id=768362
Based-on-patch-by: gordonb3 <gordon@bosvangennip.nl>
We use the merge function to initialize the cloned instance.
Previously, merge did not always copy all properties, so the
cloned instance might not have been identical. Fix that.
gboolean is a typedef for int, so there is no difference in behavior.
However, we use IS_IPv4 as index into arrays of length two. Making
it "int" seems more approriate. Also, this is what all the other
(similar) code does.
dhcp-anycast-address is only set by OLPC mesh device. It's ugly to have
this in form of a nm_device_set_dhcp_anycast_address() method, because
that means to cache the address in NMDevice. Meaning, we have more state
in NMDevice, where it's not clear where it comes from.
Instead, whenever we need to DHCP anycast address, as the subclass to
provide it (if any). This way, it gets extracted from the currently
applied connection at the moment when it is needed. Beyond that, the
setting is not duplicated/cached in NMDevice anymore.
Instead of passing the setting on during ip4_start()/ip6_start(), make
it a property of NMDhcpClient.
This property is currently only set by OLPC devices, and is only
implemented by NMDhcpDhclient. As such, it also does not need to change
or get reset. Hence, and immutable, construct-only property is clearer,
because we don't have to pass parameters to ip[46]_start().
Arguably, the parameter is still there, but being immutable and always
set, make it easier to reason about it.
Kernel will coerce values like
ethtool -A eth0 autoneg on rx off
to have autonet still on.
Also, if autoneg on the interface is enabled, then `ethtool -A eth0 tx off`
has no effect.
In NetworkManager, the user cannot configure "autoneg on" together with
any rx/tx settings. That would render the profile invalid. However, we
also need to take care that a profile
nmcli connection add ... ethtool.pause-autoneg ignore ethtool.pause-tx off
really means off. That means, we must coerce an unspecified autoneg
setting to "off".
Setting pause-rx/pause-tx to an explicit value, implies that the user
does not want to enable autoneg. Reject that as invalid value in the
connection profile.
Currently we commit the MTU to the device when updating the IP
configuration, or when a port device is added to the controller. This
means that for a connection with DHCP, the MTU is set only after DHCP
has completed. In particular, if DHCP doesn't complete and the
connection has an infinite timeout, the MTU is never set.
_commit_mtu() tracks different sources for the MTU of a device, and
each source has a different priority. Among these sources there are
the parent link (for VLANs), a dynamic IP configuration (DHCP, PPP)
and the connection profile.
A MTU from the connection always has the highest priority and
overrides other sources.
Therefore, if the connection specifies an MTU it can be applied at
stage2, even before configuring IP addressing.
https://bugzilla.redhat.com/show_bug.cgi?id=1890234https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/859
If the device is still unmanaged by platform-init (which means that
udev didn't emit the event for the interface) when the device gets
realized, we currently clear the assume state. Later, when the device
becomes managed, NM is not able to properly assume the device using
the UUID.
This situation arises, for example, when NM already configured the
device in initrd; after NM is restarted in the real root, udev events
can be delayed causing this race condition.
Among all unamanaged flags, platform-init is the only one that can be
delayed externally. We should not clear the assume state if the device
has only platform-init in the unmanaged flags.
