When NMClient gets destroyed, it unrefs all NMObject. We need to unbreak
cycles then, and the property getters must return NULL. In particular,
for "o" type properties (NMLDBusPropertyO), this was not done correctly.
For example, calling nm_device_get_active_connection() while/after
destroying the NMClient can give a dangling pointer and assertion
failure. This will also be covered by test_activate_virtual(). Probably
a similar issue can happen, when a D-Bus object gets removed (without
destroying NMClient altogether).
The fix is that nml_dbus_property_o_clear() needs to clear "nmobj". That
is correct, because the pointer is no longer valid and should not be there.
And the unit test shows that in fact a pointer is left there, and
clearing it fixes it.
That was different from an earlier attempt to fix this (in commit 62b2aa85e8
('Revert "libnm: fix dangling pointer in public API while destructing NMClient"')),
where clearing the pointer at a different place broke things. That
attempt was wrong, because nml_dbus_property_o_notify_changed() needs to be the
one that sets/clears nmobj field during a regular update. But the case
here is not a regular update, nml_dbus_property_o_clear() happens during
unregister/cleanup, and then we need to clear the pointer.
Fixes: ce0e898fb4 ('libnm: refactor caching of D-Bus objects in NMClient')
https://bugzilla.redhat.com/show_bug.cgi?id=2039331https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/896
See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1064https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1075
For IPv6 the lease doesn't necessarily have an address. If the address
is missing or the DHCP client doesn't implement accept(), we don't
need to wait for the address in platform.
From https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1066#note_1233210 :
0 0x00007ffff760f88c in __pthread_kill_implementation () at /lib64/libc.so.6
1 0x00007ffff75c26a6 in raise () at /lib64/libc.so.6
2 0x00007ffff75ac7d3 in abort () at /lib64/libc.so.6
3 0x00007ffff77c5d4c in g_assertion_message (domain=<optimized out>, file=<optimized out>, line=<optimized out>, func=<optimized out>, message=<optimized out>)
at ../glib/gtestutils.c:3223
4 0x00007ffff782645f in g_assertion_message_expr
(domain=domain@entry=0x5555559e7c96 "nm", file=file@entry=0x5555559deac0 "src/core/dhcp/nm-dhcp-client.c", line=line@entry=609, func=func@entry=0x5555559e0090 <__func__.31> "l3_cfg_notify_cb", expr=expr@entry=0x5555559df5cf "lease_address") at ../glib/gtestutils.c:3249
5 0x00005555558b2866 in l3_cfg_notify_cb (l3cfg=0x555555c29790, notify_data=<optimized out>, self=0x555555e9a1b0) at src/core/dhcp/nm-dhcp-client.c:609
9 0x00007ffff791abe3 in <emit signal ??? on instance ???> (instance=instance@entry=0x555555c29790, signal_id=<optimized out>, detail=detail@entry=0) at ../gobject/gsignal.c:3553
6 0x00007ffff78fcc7f in g_closure_invoke (closure=0x555555ca3900, return_value=0x0, n_param_values=2, param_values=0x7fffffffd420, invocation_hint=0x7fffffffd3a0)
at ../gobject/gclosure.c:830
7 0x00007ffff7919106 in signal_emit_unlocked_R
(node=node@entry=0x555555bbadc0, detail=detail@entry=0, instance=instance@entry=0x555555c29790, emission_return=emission_return@entry=0x0, instance_and_params=instance_and_params@entry=0x7fffffffd420) at ../gobject/gsignal.c:3742
8 0x00007ffff791a9ca in g_signal_emit_valist (instance=<optimized out>, signal_id=<optimized out>, detail=<optimized out>, var_args=var_args@entry=0x7fffffffd5f0)
at ../gobject/gsignal.c:3497
10 0x000055555564c8a6 in _nm_l3cfg_emit_signal_notify (self=self@entry=0x555555c29790, notify_data=notify_data@entry=0x7fffffffdf30) at src/core/nm-l3cfg.c:576
11 0x000055555564ce77 in _nm_l3cfg_emit_signal_notify_simple (self=self@entry=0x555555c29790, notify_type=notify_type@entry=NM_L3_CONFIG_NOTIFY_TYPE_POST_COMMIT)
at src/core/nm-l3cfg.c:585
12 0x0000555555656082 in _l3_commit (self=self@entry=0x555555c29790, commit_type=NM_L3_CFG_COMMIT_TYPE_UPDATE, commit_type@entry=NM_L3_CFG_COMMIT_TYPE_AUTO, is_idle=is_idle@entry=1)
at src/core/nm-l3cfg.c:4201
13 0x0000555555656189 in _l3_commit_on_idle_cb (user_data=user_data@entry=0x555555c29790) at src/core/nm-l3cfg.c:2961
14 0x00007ffff77f847b in g_idle_dispatch (source=0x555555d65680, callback=0x55555565612c <_l3_commit_on_idle_cb>, user_data=0x555555c29790) at ../glib/gmain.c:5897
15 0x00007ffff77fc130 in g_main_dispatch (context=0x555555aa5020) at ../glib/gmain.c:3381
16 g_main_context_dispatch (context=0x555555aa5020) at ../glib/gmain.c:4099
17 0x00007ffff7851208 in g_main_context_iterate.constprop.0 (context=0x555555aa5020, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4175
18 0x00007ffff77fb853 in g_main_loop_run (loop=0x555555aa5970) at ../glib/gmain.c:4373
19 0x0000555555593c56 in main (argc=<optimized out>, argv=<optimized out>) at src/core/main.c:509
Fixes: e1648d0665 ('core: commit l3cd asynchronously on DHCP bound event')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1074
Update DNS only when something relevant changes:
- an old l3cd gets removed, without adding a new one
- a new one is added without removing an old one
- an old is removed and it differs (in routes and DNS) from the new
added one
NMPolicy stores the last hostname set in priv->cur_hostname and checks
if it changed before notifying the DNS manager.
_set_hostname() should be called one time early during startup so that
the priv->cur_hostname gets initialized without triggering a DNS
update.
I see a significant performance increase with many parallel
activations if DNS updates are deferred to the SECONDARIES state.
Since there is no guarantee that device_l3cd_changed() is called again
when the device becomes ACTIVATED, we need also to change
device_state_changed().
When a lease is obtained, currently NMDevice performs a synchronous
commit of IP configuration and then accepts the lease.
Instead, let NMDevice only schedule a async commit; when the DHCP
client notices that the new address was committed it will
automatically accept it and emit a new signal so that the device can
succeed the activation.
Sync commits should be avoided because a commit does of things which
are outside the control of the caller (see the comment in
nm_device_l3cfg_commit()). Furthermore, when there are many pending
activations, async commits seem to help in reducing the CPU usage.
While making the commit async, also move the responsibility of the
accept() to NMDhcpClient.
This breaks test @nmcli_monitor. With this patch, `nmcli monitor` no
longer prints "There's no primary connection". Need to investigate why.
For now, revert the patch.
This reverts commit 2afecaf908.
Currently, ip4 new config signal is being emitted twice, due
to the lack of a barrier to a possible situation. This
commit includes this.
This fixes a crash on NMCI tests due to an assertion failure,
now it will not go ahead in the function anymore if it is under the
undesired conditions.
The backtrace of the crashes can be found at
https://bugzilla.redhat.com/show_bug.cgi?id=2028385
Previously (on RHEL<=8 and Fedora<=35), NetworkManager package contains
the compat scripts nm-ifup/nm-ifdown.
If initscripts package (not network-scripts!) is installed, then a RPM
trigger links them as alternatives for the ifup/ifdown commands.
One problem is that `dnf provides /usr/sbin/ifup` lists the
NetworkManager package. Which is technically true, but on RHEL9 where
initscripts is not installed by default, `dnf install NetworkManager`
does not actually create those scripts.
Solve that by moving those scripts to a new subpackage
NetworkManager-initscripts-updown. The %post script now always creates the
alternatives links, regardless whether initscripts package is installed.
Note that on RHEL8, NetworkManager package not only Obsoletes: but also
Suggests: the new package.
The name "initscripts-updown" is chosen because in the future we might
have additonal initscripts/ifcfg related subpackages to contain the
ifcfg-rh plugin (NetworkManager-initscripts-ifcfg) or ifcfg-rh migration
tools (NetworkManager-initscripts-tools).
https://bugzilla.redhat.com/show_bug.cgi?id=2022418https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1061
While (and after) NMClient gets destroyed, nm_device_get_active_connection()
gives a dangling pointer. That can lead to a crash. This probably
affects all NMLDBusPropertyO type properties.
It's not clear how to fix that best. Usually, NMClient does updates in
two phases, first it processes the D-Bus events and tracks internal
data, then it emits all GObject signals and notifications.
When an object gets removed from the NMClient cache, then the second
phase is not fully processed, because the object is already removed
from the cache. Thus, the property was not properly cleared leaving
a dangling pointer.
A simple fix is to always clear the pointer during the first phase. Note that
effectively we do the same also for NMLDBusPropertyAO (by clearing the
"pr_ao->arr"), so at least this is consistent.
Somehow it seems that we should make sure that the "second" phase gets
full processed in this case too. But it's complicated, and it's not
clear how to do that. So this solution seems fine.
https://bugzilla.redhat.com/show_bug.cgi?id=2039331https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/896
When destroying NMClient, nm_device_get_active_connection() still
return dangling pointers. Add a unit test for that bug.
Obviously, the bug currently exists, so the relevant code is commented
out.
Why? Because I often use a command line like
$ ./tools/run-nm-test.sh -m src/libnm-client-impl/tests/test-nm-client -p /libnm/device-connection-compatibility
or even alias it to a one character command `x`.
Usually I want to do the rebuild, and as `make` is so slow, it
adds noticeable time running the command. Thus, sometimes I want
to modify the command, for which I have to edit the command from the
history, or toggle two separate commands.
Add a `-M` flag that can reverse the effect of an earlier `-m`.
An "enable" flag in general should just also have a "disable" flag.
In function 'nm_uuid_unparse',
inlined from 'nm_uuid_generate_from_string_str' at src/libnm-glib-aux/nm-uuid.c:393:12,
inlined from 'nm_uuid_generate_from_strings.constprop' at src/libnm-glib-aux/nm-uuid.c:430:16:
src/libnm-glib-aux/nm-uuid.h:37:12: error: 'uuid' may be used uninitialized [-Werror=maybe-uninitialized]
37 | return nm_uuid_unparse_case(uuid, out_str, FALSE);
| ^
src/libnm-glib-aux/nm-uuid.c: In function 'nm_uuid_generate_from_strings.constprop':
src/libnm-glib-aux/nm-uuid.c:20:1: note: by argument 1 of type 'const struct NMUuid *' to 'nm_uuid_unparse_case.constprop' declared here
20 | nm_uuid_unparse_case(const NMUuid *uuid, char out_str[static 37], gboolean upper_case)
| ^
src/libnm-glib-aux/nm-uuid.c:390:12: note: 'uuid' declared here
390 | NMUuid uuid;
| ^
lto1: all warnings being treated as errors
The problem are code paths with failed g_return*() assertions. Being in
a bad state already, they don't bother to ensure proper return values,
and with LTO the compiler might think there are valid code paths wrongly
handled. Work around.
If the connection has wfd_ies set, we want to ensure that a WFD
connection is being established so we want to check that the target peer
also supports WFD. For now this in the IWD backend only.
However, WFD peers will send us their WFD IEs in their Probe Responses
only if our own Probe Request contained a WFD IE, or at least that's
when the spec mandates that they include the WFD IE. This implies that
the discovery phase (NM.Device.WifiP2P.StartFind(options) / .StopFind())
needs to know the value of the local WFD IE and include it in the Probe
Requests, which existing clients (gnome-network-displays) doesn't do and
in fact can't do because there's no API for the client to pass the WFD
IE -- that is easy to add in the options parameter though (a
dictionary).
The current situation is that with the wpa_supplicant backend we'll
sometimes see the WFD IE (when the peer is discovered from a Probe
Request that it has sent, rather than from a Probe Response) and
sometimes not, while with the IWD backend we'll never see the WFD
information because IWD assumes that the client (NM) is not interested
in seeing it if it hasn't registered the local WFD information before
starting discovery.
So, for compatibility with this existing situation and with the
wpa_supplicant backend, ignore whether the peer is a WFD peer except in
the PREPARE stage. In PREPARE, if we have a peer compatible with the
connection being activated -- except for the WFD information -- force a
new discovery, this time passing the WFD information from the
NMSettingConnection's wfd_ies, and only progress to CONFIG if the target
peer is re-discovered as a WFD peer within 10 seconds.
We don't actually check the contents of the WFD IEs to match, e.g. we
don't check the sink/source/dual role compatibility between our and the
peer's properties, but IWD will do some of these checks later during
activation.
Similarly as with wpa_supplicant, create NMDeviceIwdP2P objects for P2P
devices based on data from IWD -- not in NMWifiFactory::create_device
because that is only triggered for system netdevs and a P2P-Device
virtual interface has no netdev. Unlike NMDeviceWifiP2P,
NMDeviceIwdP2P's iface property is a unique string that likely doesn't
match any system interface name -- in theory there doesn't need to be
any related netdev on the system (such as an Infrastructure-mode
interface) before a P2P-Device is added.
[thaller@redhat.com: modified original patch]
Since the NM D-Bus API talks to the client using raw WFD IE bytes and
IWD's API wants/provides some of the actual values from the WFD IE
instead (e.g. boolean Source and Sink properties), we need to be able to
parse and build the WFD IE from those property values to do the
translation, add utilities for this. Use one of them to build the WFD IE
property on NMWifiP2PPeer objects.
[thaller@redhat.com: modify original patch to use
nm_g_variant_unref() and add missing #define]
In NMWifiFactory, move the check that prevents NMDevice's from being
created for interfaces in Monitor and other modes, out of the
wpa_supplicant-specific block so that it applies to both IWD and
wpa_supplicant. This check allows P2P device creation to work
differently that Infrastructure, Ad-Hoc and AP modes so it's needed for
P2P support in the IWD backend.
While there change the check to list the modes that are accepted rather
than rely on some of the modes (Monitor, P2P-{Device,Client,GO}) being
bunched together as _NM_802_11_MODE_UNKNOWN.
The interface name has changed in 2019 but the WSC interface has
never been used by NM. Update #define NM_IWD_WSC_INTERFACE in
nm-iwd-manager.h accordingly.
In function '_nm_auto_g_free',
inlined from 'test_tc_config_tfilter_matchall_mirred' at src/libnm-core-impl/tests/test-setting.c:2955:24:
./src/libnm-glib-aux/nm-macros-internal.h:58:1: error: 'str' may be used uninitialized [-Werror=maybe-uninitialized]
58 | NM_AUTO_DEFINE_FCN_VOID0(void *, _nm_auto_g_free, g_free);
| ^
src/libnm-core-impl/tests/test-setting.c: In function 'test_tc_config_tfilter_matchall_mirred':
src/libnm-core-impl/tests/test-setting.c:2955:24: note: 'str' was declared here
2955 | gs_free char *str;
| ^
lto1: all warnings being treated as errors
lto-wrapper: fatal error: gcc returned 1 exit status
