GByteArray is a mutable array of bytes. For every practical purpose, the hwaddr
property of NMDhcpClient is an immutable sequence of bytes. Thus, make it a
GBytes.
The two boolean properties do not need to be ever reset. It's nice
to initialize such properties in the constructor and don't mutate
them afterwards.
Instead of adding two boolean GObject properties, add a new flags property
that can encode these two values. In the end, properties are too
cumbersome, let's combine them.
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)
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
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.
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.
dad6_failed_addrs is populated with addresses from the platform cache.
Inside the cache, all addresses have addr_source NM_IP_CONFIG_SOURCE_KERNEL,
because addr_source property for addresses is only a property that is
used NetworkManager internally.
NMPObjects are never modified after being put into the cache.
Hence, it is safe and encouraged to just keep a reference to them,
instead of cloning them.
Interestingly, NMPlatform's change signals have a platform_object
pointer, which is not the pointer to the NMPObjects itself, but
down-cast to the NMPlatformObject instance. It does so, because commonly
callers want to have a pointer to the NMPlatformObject instance, instead
of the outer NMPObjects. However, NMP_OBJECT_UP_CAST() is guaranteed
to work one would expect.
The order in which we add addresses to dad6_failed_addrs does not
matter. Hence, use g_slist_prepend() which is O(1), instead
g_slist_append() with O(n).
We want to add addresses in a particular order so that source address
selection works.
Note that @known_addresses contains the desired addresses in order of
least-important first, while @plat_addresses contains them in opposite
order. Previously, this inverted order was not considered, and we
essentially ended up removing and re-adding all addresses every time.
Fix that. While at it, get rid of the O(n^2) runtime complexity, and
make it O(n) by iterating both lists simultaneously.
Temporary addresses (RFC4941) are not handled by NetworkManager directly, but by
kernel. If they are in the @known_addresses list, clear them out early.
They shall be ignored.
Often, we want in API that an input argument is read-only and not modified
by the function call. Not modifying input arguments is a good
convention.
However, in this case there are only two callers, and both clearly do
not care whether the @known_addresses array will be modified.
Clear out addresses that are already expired and enforce that there are
no duplicate addresses. Basically, use @known_addresses for bookkeeping
which addresses are to be ignored.
We do a pre-run that constructs an index of all addresses and drops
addresses that are already expired.
Move this code to a separate function, it will be reused for IPv6.
Also, note that nm_platform_ip4_address_sync() has only 2 callers. Both
callers make sure to not pass duplicate known addresses, because the
addresses also come from a cache. Make that a requirement and assert
against unique addresses. If we would allow duplicate addresses, we would
have to handle them in a defined way (like, dropping the ones with lower
priority). That would be more complicated, and since no caller is
supposed to provide duplicate addresses, don't bother but assert.
nm_utils_lifetime_get() already has so many arguments.
Essentially, the function returned %TRUE if and only if the
lifetime was greater then zero.
Combine the return value and the output argument for the lifetime.
It also matches better the function name: to get the lifetime.
Originally, the platform cache did not preserve any stable order.
We added that during the large cache rework. However, we still
would only care about a particular ordering for route's BY_WEAK_ID
index. For all other indexes, it was sufficient to have the
object in some arbitrary order, not necessarily the one as indicated by
kernel.
However, for addresses we actually care about the order (at least,
regarding the the OBJECT_BY_IFINDEX index, which is considered by
platform's address sync).
During a dump we get all objects in the right order. That means,
as we (re) insert the objects into the cache, we must forcefully move
them to the end of their list.
If the object didn't actually change, previously we would not have
updated their position in the cache. Fix that now.
Add a function that allows to re-request all objects of a certain type.
Usually, the cache is supposed to keep itself in a consistent state and
this function is not useful.
It is however useful during testing and debugging to explicitly reload
an object type.
If you ever think to need this function in non-testing code, then
something else is probably wrong with the cache implementation.
In ndisc_set_router_config(), we initialize NMNDiscAddress based on
NMPlatformIP6Address instances. Note that their handling of timestamps
is not entirely identical.
For convenience of the user, NMPlatformIP6Address allows to not specify
any timestamp. On the contrary, for convenience of implementation does
NMNDiscAddress always require fully specified timestamps.
Properly convert one representation into the other.
Previously we always updated resolv.conf on quit. When we are using
systemd-resolved the update is not necessary because the resolver on
127.0.0.53 would still be reachable after NM quits. Also, when NM
manages resolv.conf directly there is no need to update the file
again. Let's rewrite resolv.conf only when using dnsmasq.
https://bugzilla.redhat.com/show_bug.cgi?id=1541031
This makes its prototype compatible with GDestroyNotify so that GCC 8.0
won't warn.
The return value is not used anywhere and the unref() functions typically
don't return any.
Fixes the following error when building with gcc 4.8.5 and address
sanitizer:
src/dns/nm-dns-dnsmasq.c: In function 'update':
src/dns/nm-dns-dnsmasq.c:506:44: error: 'first_prio' may be used uninitialized in this function [-Werror=maybe-uninitialized]
} else if (first_prio < 0 && first_prio != prio)
^
