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.
Remove the redundant action argument. It is clear, that
nm_dispatcher_call_connectivity() is called with action
NM_DISPATCHER_ACTION_CONNECTIVITY_CHANGE.
On the other hand, add the async callbacks. Altough they are
not used at the moment, it seems more correct that an async
API has a callback and a call-id to cancel the invocation.
- use _NM_GET_PRIVATE() and _NM_GET_PRIVATE_PTR() everywhere.
- reorder statements, to have GObject related functions (init, dispose,
constructed) at the bottom of each file and in a consistent order w.r.t.
each other.
- unify whitespaces in signal and properties declarations.
- use NM_GOBJECT_PROPERTIES_DEFINE() and _notify()
- drop unused signal slots in class structures
- drop unused header files for device factories
In order to pass the connectivity state to the relevant hooks along with
the event itself, we need to add this parameter for the 'Action' method
of then internal 'org.freedesktop.nm_dispatcher' interface, which will
be sent by the network manager main process to the dispatcher.
https://bugzilla.gnome.org/show_bug.cgi?id=768969
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
The logging macros _LOGD(), etc. are specific to each
file as they format the message according to their context.
Still, they were cumbersome to define and their implementation
was repeated over and over (slightly different at times).
Move the declaration of these macros to "nm-logging.h".
The source file now only needs to define _NMLOG(), and either
_NMLOG_ENABLED() or _NMLOG_DOMAIN.
This reduces code duplication and encourages a common implementation
and usage of these macros.
Rather than randomly including one or more of <glib.h>,
<glib-object.h>, and <gio/gio.h> everywhere (and forgetting to include
"nm-glib-compat.h" most of the time), rename nm-glib-compat.h to
nm-glib.h, include <gio/gio.h> from there, and then change all .c
files in NM to include "nm-glib.h" rather than including the glib
headers directly.
(Public headers files still have to include the real glib headers,
since nm-glib.h isn't installed...)
Also, remove glib includes from header files that are already
including a base object header file (which must itself already include
the glib headers).
Manual page claims that a missing configuration option for connectivity
interval means "300". That was not the case for a long time (never?).
https://bugzilla.gnome.org/show_bug.cgi?id=723350
Based-on-patch-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>
There won't be any further requests, so there's no point in keeping
the connection alive. Even if the HTTP server doesn't care, proxy
servers in-between might keep the connection open for a couple seconds
for keepalive, and we might as well be nice to them and tell them we
don't need to keep it alive.
==5177== 104 (+104) bytes in 1 (+1) blocks are definitely lost in loss record 5,502 of 6,581
==5177== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==5177== by 0x7F4A6F5: g_malloc (gmem.c:97)
==5177== by 0x7F6159F: g_slice_alloc (gslice.c:1007)
==5177== by 0x7F61B6D: g_slice_alloc0 (gslice.c:1032)
==5177== by 0x7CDB9A3: g_type_create_instance (gtype.c:1847)
==5177== by 0x7CBF356: g_object_new_internal (gobject.c:1774)
==5177== by 0x7CC0D4C: g_object_newv (gobject.c:1922)
==5177== by 0x7CC14E3: g_object_new (gobject.c:1614)
==5177== by 0x79A4C57: g_simple_async_result_new (gsimpleasyncresult.c:319)
==5177== by 0x515B34: nm_connectivity_check_async (nm-connectivity.c:289)
==5177== by 0x515D74: run_check (nm-connectivity.c:217)
==5177== by 0x515DBF: idle_start_periodic_checks (nm-connectivity.c:229)
