Optimally, NMDhcpClient would be stateless and all paramters would
be passed on as argument. Clearly that is not feasable, because there
are so many paramters, and in many cases they need to be cached for the
lifetime of the client instance.
Instead of passing info_only paramter to ip6_start() and cache it
both in NMDhcpClient and NMDhcpSystemd, keep it in NMDhcpClient at
one place.
In the next commit, we will initialize info-only only once during the
constructor, so it is immutable and somewhat stateless.
The parent's stop() implementation does nothing interesting
for NMDhcpSystem. Still, call it, it's just unexpected to
not chain up the parent implementation, if all other subclasses
do it.
In general, if the parent's implementation is not suitable to be called
by the derived class, that should be handled differently then just not
chaining up. Otherwise it's inconsistent and confusing.
Don't use message data after calling curl_multi_remove_handle(). Fixes
the following asan error:
=================================================================
==13238==ERROR: AddressSanitizer: heap-use-after-free on address 0x608000091ad0 at pc 0x55750f8d9a10 bp 0x7ffeb7f5f210 sp 0x7ffeb7f5f200
READ of size 8 at 0x608000091ad0 thread T0
#0 0x55750f8d9a0f in curl_check_connectivity (/usr/sbin/NetworkManager+0x190a0f)
#1 0x55750f8da7dd in curl_socketevent_cb (/usr/sbin/NetworkManager+0x1917dd)
#2 0x7f73cb64e8f8 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4a8f8)
#3 0x7f73cb64ec57 (/lib64/libglib-2.0.so.0+0x4ac57)
#4 0x7f73cb64ef29 in g_main_loop_run (/lib64/libglib-2.0.so.0+0x4af29)
#5 0x55750f85c3f4 (/usr/sbin/NetworkManager+0x1133f4)
#6 0x7f73c9f19384 in __libc_start_main (/lib64/libc.so.6+0x22384)
#7 0x55750f85d7f7 (/usr/sbin/NetworkManager+0x1147f7)
0x608000091ad0 is located 48 bytes inside of 88-byte region [0x608000091aa0,0x608000091af8)
freed by thread T0 here:
#0 0x7f73cd61f508 in __interceptor_free (/lib64/libasan.so.4+0xde508)
#1 0x7f73ca710eaa in curl_multi_remove_handle (/lib64/libcurl.so.4+0x32eaa)
previously allocated by thread T0 here:
#0 0x7f73cd61fa88 in __interceptor_calloc (/lib64/libasan.so.4+0xdea88)
#1 0x7f73ca710b3d in curl_multi_add_handle (/lib64/libcurl.so.4+0x32b3d)
SUMMARY: AddressSanitizer: heap-use-after-free (/usr/sbin/NetworkManager+0x190a0f)
Disable undefined sanitizer on RHEL since it's not supported. Also,
enable address sanitizer only for executables, as having it enabled in
libraries causes problems when applications built without asan load
them.
The address sanitizer is not compatible [1] with libraries dynamically
opened using RTLD_DEEPBIND: disable the flag when building with asan.
[1] https://github.com/google/sanitizers/issues/611
Shared libraries built with sanitizers are a bit inconvenient to use
because they require that any application linking to them is run with
libasan preloaded using LD_PRELOAD. This limitation makes the
sanitizer support less useful because applications will refuse to
start unless there is a special environment variable set.
Let's turn the --enable-address-sanitizer configure flag into
--with-address-sanitizer=yes|no|exec so that is possible to enable
asan only for executables.
After writing the connection to disk and rereading it, in addition to
restoring agent-owned secrets in the cache we must also restore
agent-owned secrets from the original connections since they are lost
during the write.
Reported-by: Märt Bakhoff <anon@sigil.red>
https://bugzilla.gnome.org/show_bug.cgi?id=793324
Let the matching continue when we are autocompleting arguments and we
have already found 'id', 'uuid' or 'path'.
Before:
# nmcli connection modify path<TAB>
path
After:
# nmcli connection modify path<TAB>
path
pathfinder-wifi
'help' is completed without considering other alternatives:
# nmcli connection modify h<TAB>
help
After the patch:
# nmcli connection modify h<TAB>
help
home-wifi
Fixes: 29bb6ae4fe
It looks bad and makes everyone super-sad:
$ nmcli --ask c modify 'Oracle HQ' 802-11-wireless-security.psk solaris666
System policy prevents modification of network settings for all users
(action_id: org.freedesktop.NetworkManager.settings.modify.system)
Password (lkundrak): *********
$
With --ask it might call back to nmcli's agent, causing a deadlock
while the client is waiting for the response. Let's give the client
a chance to service the agent requests while waiting:
$ nmcli --ask --show-secrets c show 'Oracle HQ'
<hang>
This is probably still rather suboptimal and inefficient, since we
still serialize the calls and block on response. However, if we submit
multiple calls to GetSecrets, the daemon would start authorizing the
first one and fail the other ones immediately before the authorization
succeeds.
This could perhaps be addressed in the daemon, but let's settle for a
fix that's compatible with the current daemon for now.
The bridge test (and no other either) no longer sets sysfs properties,
so this whole madness is no longer needed. That is good, because Linux
got somewhat stricter (at least in 4.15) about mounting sysfs and the
whole thing wouldn't work with containers where /sys is red-only from
the start.
If we're going to use a 'no content' URL (HTTP 204) to check
connectivity, do not try to match prefix when the content is being
received. This issue was making the check not work properly, as the content
returned by the captive portal was assumed as expected (given that
g_str_has_prefix(str,"") always returns TRUE!).
Also, rework a log message that was being emitted on portal detection
to avoid specifying that the reason is the content being shorter than
expected, as that same logic now applies to the case where too much
content is received and none was expected.
Fixes: 88416394f8https://mail.gnome.org/archives/networkmanager-list/2018-February/msg00009.html
The ifcfg-rh plugin provides its own D-Bus service which initscripts
query to determine whether NetworkManager handles an ifcfg file.
Rework the D-Bus glue to hook GDBus with NetworkManager to use
GDBusConnection directly. Don't use generated code, don't use
GDBusInterfaceSkeleton.
We still keep "src/settings/plugins/ifcfg-rh/nm-ifcfg-rh.xml"
and still compile the static generated code. We don't actually need
them anymore, maybe the should be dropped later.
This is a proof of concept for reworking the D-Bus glue in
NetworkManager core to directly use GDBusConnection. Reworking core is
much more complicated, because there we also have properties, and a
class hierarchy.
Arguably, for the trivial ifcfg-rh service all this hardly matters, because
the entire D-Bus service only consists of one method, which is unlikely to
be extended in the future.
Now we get rid of layers of glue code, that were hard to comprehend.
Did you understand how nm_exported_object_skeleton_create() works
and uses the generated code and GDBusInterfaceSkeleton to hook into
GDBusConnection? Congratulations in that case. In my opinion, these
layers of code don't simplify but complicate the code.
The change also reduces the binary size of "libnm-settings-plugin-ifcfg-rh.so"
(build with contrib/rpm --without debug) by 8312 bytes (243672 vs. 235360).
In _dbus_setup(), call _dbus_clear(). It feels more correct to do that.
Although, technically, we never even call _dbus_setup() if there is
anything to clear.
Also, minor refactoring of config_changed_cb(). It's not entirely clear
whether we need that code, or how to handle D-Bus disconnecting, if at all.
Add macros that cast away the constness of a pointer, but
ensure that the type of the pointer is as expected.
Unfortunately, there is no way (AFAIK) to remove the constness of
a variable, without explicitly passing @type to the macro.
Substrings matching the heading of valid values were allowed if not
ambiguous (e.g.: "et" for "eth"). Moreover, upper case variants were
accepted too.
Do a plain string comparison check against the valid values.
Improve also the error message: give a list of valid tx-hashes.
when input matched the heading of two allowed values the match was
reported as ambiguous without checking if there was a perfect match
following: fixed.
Example of a failing input:
const char **allowed = [ "ipv4, ipv6, ip" ];
const char *input = "ip";
"ip" was detected as ambiguous.
Avoid calling nm_device_iwd_set_dbus_object (device, NULL) if the
dbus_object was NULL already. Apparently gdbus guarantees that a
name-owner notification either has a NULL old owner or a NULL new owner
but can also have both old and new owner NULL.
Reuse the apparent workaround from libnm/nm-client.c in which the
GDbusObjectManagerClient is recreated every time the name owner
pops up, instead of creating it once and using that object forever.
Resubscribe to all the signals on the new object. The initial
GDbusObjectManager we create is only used to listed for the name-owner
changes.
There's nothing in gdbus docs that justifies doing that but there
doesn't seem to be any way to reliably receive all the signals from
the dbus service the normal way. The signals do appear on dbus-monitor
and the gdbus apparently subscribes to those signals with AddMatch()
correctly but they sometimes won't be received by the client code,
unless this workaround is applied.
While making changes to got_object_manager, don't destroy the
cancellable there as it is supposed to be used throughout the
NMIwdManager life.
Disconnect from NMManager signals in our cleanup, make sure the
NMManager singleton is not destroyed before we are by keeping a
reference until we've disconnected from its signals.
Add very simple periodic scanning because IWD itself only does periodic
scanning when it is in charge of autoconnecting (by policy). Since we
keep IWD out of the autoconnect state in order to use NM's autoconnect
logic, we need to request the scanning. The policy in this patch is to
use a simple 10s period between the end of one scan the requesting of
another while not connected, and 20s when connected. This is so that
users can expect similar results from both wifi backends but without
duplicating the more elaborate code in the wpa_supplicant backend which
can potentially be moved to a common superclass.
Although IFA_F_TEMPORARY is numerically equal to IFA_F_SECONDARY,
their meaning is different. One applies to IPv6 temporary addresses,
and the other to IPv4 secondary addresses.
During _addr_array_clean_expired() we want to ignore and clear
IPv6 temporary addresses, but not IPv4 secondary addresses.
Fixes: f2c4720bca
While the numerical values of IFA_F_SECONDARY and IFA_F_TEMPORARY
are identical, their meaning is not.
IFA_F_SECONDARY is only relevant for IPv4 addresses, while
IFA_F_TEMPORARY is only relevant for IPv6 addresses.
IFA_F_TEMPORARY is automatically set by kernel for the addresses
that it generates as part of IFA_F_MANAGETEMPADDR. It cannot be
actively set by user-space.
IFA_F_SECONDARY is automatically set by kernel depending on the order
in which the addresses for the same subnet are added.
This essentially reverts 8b4f11927 (core: avoid IFA_F_TEMPORARY alias for
IFA_F_SECONDARY).
In device_ipx_changed() we remember the addresses for which it appears
that DAD failed. Later, on an idle handler, we process them during
queued_ip6_config_change().
Note that nm_plaform_ip6_address_sync() might very well decide to remove
some or all addresses and re-add them immidiately later. It might do so,
to get the address priority/ordering right. At that point, we already
emit platform signals that the device disappeared, and track them in
dad6_failed_addrs.
Hence, later during queued_ip6_config_change() we must check again
whether the address is really not there and not still doing DAD.
Otherwise, we wrongly claim that DAD failed and remove the address,
generate a new one, and the same issue might happen again.
