The settings of NMConnectivity can change any time, by reloading the
configuration.
When reloading the configration, we don't want to interrupt or cancel
the pending reuqests, they should just complete with the old settings with
which they started. Note, that NMDevice is smart enough, that when a
newer request completes earlier, it invalidates all older, still pending
requests.
Anyway, that means, we cannot rely on the value to stay alive. Fix that,
by adding adding a new ref-counted struct for these parameters.
Fixes: 2cec94bacc
During a config change notification, we determine a "changed" value,
to know whether things significantly changed.
Also, we want to log a warning about invalid configuration,
only when the config actually changed. Previously, when the URI was
invalid, on every reload (SIGHUP) we would log an error message,
even if the configuration did not change. There is no need to
warn multiple times about the same thing.
Keep track of the original URI in priv->uri. Whenever that changed,
we know the user reconfigured something. But also, now the URI might
be set to an invalid value. That means, we need to remember whether
the URI is valid.
Also, log a warning if we fail to parse the host and port. Already
before, such an URI was considered invalid and we would effectively
not to connectivity checking.
- use cleanup attribute except explicit free/unref.
- check that the result has the expected address family.
Arguably, it should always, unless there is a bug in systemd-resolved.
By that reasoning, we also wouldn't have to check the address length
either.
- don't use strndup() for values that are later freed by g_free().
We should always agree whether to malloc/free or g_malloc/g_free.
- don't use strcasecmp(). Always use the locale independent g_ascii_strcasecmp()
instead.
- use nm_utils_inet_ntop() instead of inet_ntop(). It's our preferred
wrapper, which as a stricter semantic (for example, it cannot fail
and it's input arguments are stricter defined).
- use nm_clear_g_free() instead of g_clear_pointer().
If the user disabled systemd-resolved, two things seem apparent:
- the user does not want us to use systemd-resolved
- NetworkManager is not pushing the DNS configuration to
systemd-resoved.
It seems to me, we should not consult systemd-resolved in that case.
While at it, replace "AF_INET" with "IPv". The connectivity check
logging line is already much too long. Save a few characters. Also,
I think the meaning of "AF_INET" is less clear (to a novice user) than
"IPv4".
This is dead ugly and works around what probably is a libcurl (7.59) bug:
It shares the resolver cache among easy handles in a multi handle when
it shouldn't:
When we create two easy handles, one for IPv4 and one for IPv6 in a
single mhandle:
curl_easy_setopt (ehandle6, CURLOPT_RESOLVE,
"fedoraproject.org:2605:bc80:3010:600:dead:beef:cafe:feda"
"fedoraproject.org:2604:1580:fe00:0:dead:beef:cafe:fed1")
curl_easy_setopt (ehandle4, CURLOPT_RESOLVE,
"fedoraproject.org:8.43.85.67"
"fedoraproject.org:152.19.134.198")
curl_multi_add_handle (mhandle, ehandle6);
curl_multi_add_handle (mhandle, ehandle4);
Both end up connecting to the same (either v4 or v6) address. None of
CURLOPT_FORBID_REUSE(3), CURLOPT_FRESH_CONNECT(3),
CURLOPT_DNS_USE_GLOBAL_CACHE(3) and CURLOPT_DNS_CACHE_TIMEOUT(3) make
any difference. Nor does CURLOPT_DNS_LOCAL_IP[46].
Nothing changes practically, as the NMDevice still starts this with
AF_UNSPEC. That is going to change in the following commit.
The ugly part:
priv->concheck_x[0] in few places. I believe we shouldn't be using union
aliasing here, and instead of indexing the v4/v6 arrays by a boolean it
should be an enum. I'm not fixing it here, but I eventually plan to if
this gets an ACK.
This allows us to use the correct DNS server for the particular interface
independent of what the system resolver is configured to use.
The ugly part:
Unfortunately, it is not all that easy. The libc's libresolv.so API does
not provide means for influencing neither interface nor name servers used
for DNS resolving.
Curl can also be compiled with c-ares resolver backend that does provide
the necessary functionality, but it requires and extra library and the
Linux distributions don't seem to enable it. (Fedora doesn't, which is a
good sign we don't have an option of relying on it.)
systemd-resolved does provide everything we need. If we take care to
keep its congfiguration up to date, we can use it to do the resolving on
a particular interface with that interface's DNS configuration. Great!
There's one more problem: Curl doesn't provide callbacks for resolving
host names. It doesn't, however, allow us to pass in the pre-resolved
hostnames in form of an CURLOPT_RESOLVE(3) option. This means we have to
parse the host name out of the URL ourselves. Fair enough I guess...
libcurl does not allow removing easy-handles from within a curl
callback.
That was already partly avoided for one handle alone. That is, when
a handle completed inside a libcurl callback, it would only invoke the
callback, but not yet delete it. However, that is not enough, because
from within a callback another handle can be cancelled, leading to
the removal of (the other) handle and a crash:
==24572== at 0x40319AB: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24572== by 0x52DDAE5: Curl_close (url.c:392)
==24572== by 0x52EC02C: curl_easy_cleanup (easy.c:825)
==24572== by 0x5FDCD2: cb_data_free (nm-connectivity.c:215)
==24572== by 0x5FF6DE: nm_connectivity_check_cancel (nm-connectivity.c:585)
==24572== by 0x55F7F9: concheck_handle_complete (nm-device.c:2601)
==24572== by 0x574C12: concheck_cb (nm-device.c:2725)
==24572== by 0x5FD887: cb_data_invoke_callback (nm-connectivity.c:167)
==24572== by 0x5FD959: easy_header_cb (nm-connectivity.c:435)
==24572== by 0x52D73CB: chop_write (sendf.c:612)
==24572== by 0x52D73CB: Curl_client_write (sendf.c:668)
==24572== by 0x52D54ED: Curl_http_readwrite_headers (http.c:3904)
==24572== by 0x52E9EA7: readwrite_data (transfer.c:548)
==24572== by 0x52E9EA7: Curl_readwrite (transfer.c:1161)
==24572== by 0x52F4193: multi_runsingle (multi.c:1915)
==24572== by 0x52F5531: multi_socket (multi.c:2607)
==24572== by 0x52F5804: curl_multi_socket_action (multi.c:2771)
Fix that, by never invoking any callbacks when we are inside a libcurl
callback. Instead, the handle is marked for completion and queued. Later,
we complete all queue handles separately.
While at it, drop the @error argument from NMConnectivityCheckCallback.
It was only used to signal cancellation. Let's instead signal that via
status NM_CONNECTIVITY_CANCELLED.
https://bugzilla.gnome.org/show_bug.cgi?id=797136https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1792745https://bugzilla.opensuse.org/show_bug.cgi?id=1107197https://github.com/NetworkManager/NetworkManager/pull/207
Fixes: d8a31794c8
It seems, curl_multi_socket_action() can fail with
connectivity check failed: 4
where "4" means CURLM_INTERNAL_ERROR.
When that happens, it also seems that the file descriptor may still have data
to read, so the glib IO callback _con_curl_socketevent_cb() will be called in
an endless loop. Thereby, keeping the CPU busy with doing nothing (useful).
Workaround by disabling polling on the file descriptor when something
goes wrong.
Note that optimally we would cancel the affected connectivity-check
right away. However, due to the design of libcurl's API, from within
_con_curl_socketevent_cb() we don't know which connectivity-checks
are affected by a failure on this file descriptor. So, all we can do
is avoid polling on the (possibly) broken file descriptor. Note that
we anyway always schedule a timeout of last resort for each check. Even
if something goes very wrong, we will fail the check within 15 seconds.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=903996
On non-Windows, libcurl's "curl_socket_t" type is just a typedef for
int. We rely on that, because we use it as file descriptor.
Add a compile time check to ensure that.
Since this is "C" there are not namespaces and libraries commonly choose
a particular name prefix for their symbols.
In case of libcurl, that is "curl_".
We should avoid using the same name prefix, and choose something distinct.
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
It might happen, that connectivitiy is lost only for a moment and
returns soon after. Based on that assumption, when we loose connectivity
we want to have a probe interval where we check for returning
connectivity more frequently.
For that, we handle tracking of the timeouts per-device.
The intervall shall start with 1 seconds, and double the interval time until
the full interval is reached. Actually, due to the implementation, it's unlikely
that we already perform the second check 1 second later. That is because commonly
the first check returns before the one second timeout is reached and bumps the
interval to 2 seconds right away.
Also, we go through extra lengths so that manual connectivity check
delay the periodic checks. By being more smart about that, we can reduce
the number of connectivity checks, but still keeping the promise to
check at least within the requested interval.
The complexity of book keeping the timeouts is remarkable. But I think
it is worth the effort and we should try hard to
- have a connectivity state as accurate as possible. Clearly,
connectivity checking means that we probing, so being more intelligent
about timeout and backoff timers can result in a better connectivity
state. The connectivity state is important because we use it for
the default-route penaly and the GUI indicates bad connectivity.
- be intelligent about avoiding redundant connectivity checks. While
we want to check often to get an accurate connectivity state, we
also want to minimize the number of HTTP requests, in case the
connectivity is established and suppossedly stable.
Also, perform connectivity checks in every state of the device.
Even if a device is disconnected, it still might have connectivity,
for example if the user externally adds an IP address on an unmanaged
device.
https://bugzilla.gnome.org/show_bug.cgi?id=792240
The main issue is that `nmcli networking connectivity check` uses
nm_client_check_connectivity(), which has a timeout of 25 seconds.
Using a timeout of 30 seconds server side, means that if the requests
don't complete in time, the client side will time out and abort
with a failure. That is not right.
Fix that by using a shorter timeout server side. 20 seconds is still
plenty for a small HTTP request. If the network takes longer than that,
it's fair to call that LIMITED connectivity.
An asynchronous request should either be cancellable or not keep
the target object alive. Preferably both.
Otherwise, it is impossible to do a controlled shutdown when terminating
NetworkManager. Currently, when NetworkManager is about to terminate,
it just quits the mainloop and essentially leaks everything. That is a
bug. If we ever want to fix that, every asynchronous request must be
cancellable in a controlled way (or it must not prevent objects from
getting disposed, where disposing the object automatically cancels the
callback).
Rework the asynchronous request for connectivity check to
- return a handle that can be used to cancel the operation.
Cancelling is optional. The caller may choose to ignore the handle
because the asynchronous operation does not keep the target object
alive. That means, it is still possible to shutdown, by everybody
giving up their reference to the target object. In which case the
callback will be invoked during dispose() of the target object.
- also, the callback will always be invoked exactly once, and never
synchronously from within the asynchronous start call. But during
cancel(), the callback is invoked synchronously from within cancel().
Note that it's only allowed to cancel an action at most once, and
never after the callback is invoked (also not from within the callback
itself).
- also, NMConnectivity already supports a fake handler, in case
connectivity check is disabled via configuration. Hence, reuse
the same code paths also when compiling without --enable-concheck.
That means, instead of having #if WITH_CONCHECK at various callers,
move them into NMConnectivity. The downside is, that if you build
without concheck, there is a small overhead compared to before. The
upside is, we reuse the same code paths when compiling with or without
concheck.
- also, the patch synchronizes the connecitivty states. For example,
previously `nmcli networking connectivity check` would schedule
requests in parallel, and return the accumulated result of the individual
requests.
However, the global connectivity state of the manager might have have
been the same as the answer to the explicit connecitivity check,
because while the answer for the manual check is waiting for all
pending checks to complete, the global connectivity state could
already change. That is just wrong. There are not multiple global
connectivity states at the same time, there is just one. A manual
connectivity check should have the meaning of ensure that the global
state is up to date, but it still should return the global
connectivity state -- not the answers for several connectivity checks
issued in parallel.
This is related to commit b799de281b
(libnm: update property in the manager after connectivity check),
which tries to address a similar problem client side.
Similarly, each device has a connectivity state. While there might
be several connectivity checks per device pending, whenever a check
completes, it can update the per-device state (and return that device
state as result), but the immediate answer of the individual check
might not matter. This is especially the case, when a later request
returns earlier and obsoletes all earlier requests. In that case,
earlier requests return with the result of the currend devices
connectivity state.
This patch cleans up the internal API and gives a better defined behavior
to the user (thus, the simple API which simplifies implementation for the
caller). However, the implementation of getting this API right and properly
handle cancel and destruction of the target object is more complicated and
complex. But this but is not just for the sake of a nicer API. This fixes
actual issues explained above.
Also, get rid of GAsyncResult to track information about the pending request.
Instead, allocate our own handle structure, which ends up to be nicer
because it's strongly typed and has exactly the properties that are
useful to track the request. Also, it gets rid of the awkward
_finish() API by passing the relevant arguments to the callback
directly.
We already do conditional build with "#if WITH_CONCHECK".
Get rid of the conditional in the makefile and instead do
conditional compilating inside the source file "nm-connectivity.c".
The advantage is, now if you want to know which parts are build,
you only need to grep for the WITH_CONCHECK preprocessor define
instead of also caring about the conditional in Makefile.am and
meson.build.
It doesn't change the fact of conditional compilation. But it
consistently uses one mechanism to achieve it.
nm_connectivity_state_to_string() is entirely independent of the GObject implementation
of NMConnectivity. Move it to the beginning of the source file. It will be useful next
because we will *always* build "nm-connectivity.c" source file, but disable various
parts with #if. Hence, move the part that should always be build to the top.
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)
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
commit a955639 (connectivity: don't do periodic checks on interval=0)
broke scheduling connectivity checks.
That is because the timer is on only scheduled if
nm_connectivity_check_enabled(), which in turn only returns TRUE
if curl_mhandle is set. However, nm_connectivity_init() would only
initialize curl_mhandle after update_config(), missing to schedule
the periodic task.
https://mail.gnome.org/archives/networkmanager-list/2017-May/msg00076.html
Fixes: a95563996f
libcurl employs some typechecking via "curl/typecheck-gcc.h". When
compling with --enable-lto, compilation fails otherwise with:
make[2]: Entering directory '/data/src/NetworkManager'
CC src/src_libNetworkManager_la-nm-connectivity.lo
CCLD src/libNetworkManager.la
CCLD src/libNetworkManagerTest.la
CCLD src/dhcp/tests/test-dhcp-dhclient
src/nm-connectivity.c: In function 'curl_check_connectivity':
src/nm-connectivity.c:147:10: error: call to '_curl_easy_getinfo_err_string' declared with attribute warning: curl_easy_getinfo expects a pointer to char * for this info [-Werror]
eret = curl_easy_getinfo (msg->easy_handle, CURLINFO_PRIVATE, &cb_data);
^
lto1: all warnings being treated as errors
lto-wrapper: fatal error: /usr/bin/gcc returned 1 exit status
compilation terminated.
/usr/bin/ld: error: lto-wrapper failed
This moves tracking of connectivity to NMDevice and makes the NMManager
negotiate the best of known connectivity states of devices. The NMConnectivity
singleton handles its own configuration and scheduling of the permission
checks, but otherwise greatly simplifies it.
This will be useful to determine correct metrics for multiple default routes
depending on actual internet connectivity.
The per-device connection checks is not yet exposed on the D-Bus, since they
probably should be per-address-family as well.
CURLOPT_CONNECTTIMEOUT or CURLOPT_TIMEOUT only make sense if libcurl is
handling the I/O loop (the "easy" interface); we need to implement our
own timeout.
Factor out the conclusion of the connectivity check. This will allow us
to finish the connectivity check on other occassions than a successful
connection end. Most importantly on timeouts; but it will also allow us
to short-circuit the check when we conclude it without reading the full
response.
