If the TC setting contains no qdiscs and filters, it is lost after a
write-read cycle. Fix this by adding a new property to indicate the
presence of the (empty) setting.
NetworkManager supports a very limited set of qdiscs. If users want to
configure a unsupported qdisc, they need to do it outside of
NetworkManager using tc.
The problem is that NM also removes all qdiscs and filters during
activation if the connection doesn't contain a TC setting. Therefore,
setting TC configuration outside of NM is hard because users need to
do it *after* the connection is up (for example through a dispatcher
script).
Let NM consider the presence (or absence) of a TC setting in the
connection to determine whether NM should configure (or not) qdiscs
and filters on the interface. We already do something similar for
SR-IOV configuration.
Since new connections don't have the TC setting, the new behavior
(ignore existing configuration) will be the default. The impact of
this change in different scenarios is:
- the user previously configured TC settings via NM. This continues
to work as before;
- the user didn't set any qdiscs or filters in the connection, and
expected NM to clear them from the interface during activation.
Here there is a change in behavior, but it seems unlikely that
anybody relied on the old one;
- the user didn't care about qdiscs and filters; NM removed all
qdiscs upon activation, and so the default qdisc from kernel was
used. After this change, NM will not touch qdiscs and the default
qdisc will be used, as before;
- the user set a different qdisc via tc and NM cleared it during
activation. Now this will work as expected.
So, the new default behavior seems better than the previous one.
https://bugzilla.redhat.com/show_bug.cgi?id=1928078
Let's instead add a generic nm_device_get_ports() function.
Also, only adding new API is maybe not sufficient. We should
at the same time deprecate and alias the D-Bus API, like was done
for commit 067a3d6c08 ('nm-device: expose via D-Bus the 'hw-address'
property').
This reverts commit 754143f4e8.
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/duplicate-include-guard
Using the same include guard macro in more than one header file may
cause unexpected behavior from the compiler.
both for src/libnm-base/nm-ethtool-utils-base.h and
src/libnm-client-public/nm-ethtool-utils.h. But this is intentional,
because these two files are supposed to be identical (but compiled
twice, under different context).
Suppress the warning.
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/function-in-block
Functions should always be declared at file scope. It is confusing
to declare a function at block scope, and the visibility of the function
is not what would be expected.
Mimic the behaviour of wpa_supplicant where the "secure" identity in
TTLS and PEAP (802-1x.identity) is used as a fallback in the anonymous
identity (802-1x.anonymous_identity) if that is not provided. This is
needed to keep the profiles compatible between the two wifi backends,
for users of poorly configured WPA-Enterprise networks that require the
user login to be sent in phase 1 or in both phases.
The code responsible for this mechanism in wpa_supplicant, at the time
of writing, is
https://w1.fi/cgit/hostap/tree/src/eap_peer/eap.c?id=c733664be9dd3763c03f2da2cb32a23775dde388#n1688
and offers no comment about the privacy implications.
In two similar ``if () {} else if () {} else if () {} else {}`` sequences
the latter two {} blocks were unreachable. In the
identity/anonymous-identity case, anonymous-identity is optional,
wpa_supplicant will fall back to identity, so only check that (a likely
privacy issue because no NM or wpa_s documentation explains that the
"secure" identity is also sent in plaintext if anonymous_identity is
missing.)
In the phase2_auth/phase2_autheap case change the message to make it
clear that exactly one of the properties is expected to be present.
Drop the empty string checks because those cases is validated later in
verify() anyway.
Coverity thinks there is a problem here:
Error: TAINTED_SCALAR (CWE-20): [#def233]
NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1437: tainted_argument: Calling function "recvmsg" taints argument "msg".
NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: tainted_data: Passing tainted expression "msg.msg_controllen" to "g_realloc", which uses it as an allocation size.
NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
# 1456|
# 1457| msg.msg_controllen *= 2;
# 1458|-> msg.msg_control = g_realloc(msg.msg_control, msg.msg_controllen);
# 1459| goto retry;
# 1460| }
but the problem is not the tainted data. The problem is how should
we handle MSG_CTRUNC? If we reach MSG_CTRUNC we already lost a message.
Retrying to receive the next message is not going to fix that and is
wrong.
Also, there really is no reason why any truncation should happen. The only
ancillary data that should be present is the sender information, and for
that our buffer is supposed to be large enough.
So, simply ignore truncation. It shouldn't happen, if it happened we
cannot recover from it (aside failing an assertion), and all we really
care are the retrieved credentials. If truncation happened, we might
not have retrieved the credentials, but then that is for the caller
to handle (by rejecting the message as untrusted).
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/872
This is confusing Coverity:
Error: RESOURCE_LEAK (CWE-772): [#def249] [important]
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:810: alloc_fn: Storage is returned from allocation function "g_string_free".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:810: var_assign: Assigning: "auth_dialog_request_str" = storage returned from "g_string_free(auth_dialog_request, 0)".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:822: noescape: Resource "auth_dialog_request_str" is not freed or pointed-to in "g_output_stream_write_async".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:822: noescape: Resource "auth_dialog_request_str" is not freed or pointed-to in "g_output_stream_write_async".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:838: leaked_storage: Variable "auth_dialog_request_str" going out of scope leaks the storage it points to.
# 836| data);
# 837|
# 838|-> return TRUE;
# 839| }
# 840|
Maybe this works better to avoid the warning. At least, it also
documents it better to the reader.
The entire point of NML3ConfigData is to be immutable and merging them.
"Merging" means to combine existing settings, hence NMRefString can be
used to share the same string instance.
nettools plugin represents the way how to do it, and other plugins
should mimic that behavior. The nettools implementation adds private
DHCP options as hex, except the options
- 249 (Microsoft Classless Static Route)
- 252 (Web Proxy Auto Discovery Protocol)
Adjust systemd plugin to do the same.
For 252, we now parse the "wpad" option differently. The change in
behavior is that the property is now no longer exposed as hexstring,
but as backslash escaped plain text.
For 249, the option is not implemented. But stop adding the option as
hex-string too.
A NMRefString tracks the length seprately, it thus may not be a NUL terminated
string (although, there is always a NUL character at the end of the buffer).
As such, the previous implementation did not work correctly in when comparing
for example NMRefString("a\0b") with "a". There was even a comment hinting
to that fact. Instead of making obscure comments, fix the implementation to
behave always correctly.
This helper is useful to get a dummy GSource instance that can be
refed, unrefed and destroyed. It can act as a replacement for
a timeout source with infinite timeout.
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".
