From abd994d91646ff68a9b3769493ae8368fee2a4ea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 Jul 2018 15:02:33 +0200 Subject: [PATCH 1/4] connectivity/trivial: rename local functions to avoid "curl" prefix 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. (cherry picked from commit a24f118a1f5fae0e4e1b8ccdf044f05692b66d49) --- src/nm-connectivity.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 16c1bc3bf8..fe0c594f02 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -195,7 +195,7 @@ cb_data_free (NMConnectivityCheckHandle *cb_data, * the easy handle "at any moment"; specifically not from the * write function. Thus here we just dissociate the cb_data from * the easy handle and the easy handle will be cleaned up when the - * message goes to CURLMSG_DONE in curl_check_connectivity(). */ + * message goes to CURLMSG_DONE in _con_curl_check_connectivity(). */ curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_WRITEFUNCTION, NULL); curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_WRITEDATA, NULL); curl_easy_setopt (cb_data->concheck.curl_ehandle, CURLOPT_HEADERFUNCTION, NULL); @@ -235,7 +235,7 @@ _check_handle_get_response (NMConnectivityCheckHandle *cb_data) } static void -curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) +_con_curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) { NMConnectivityCheckHandle *cb_data; CURLMsg *msg; @@ -289,13 +289,13 @@ curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) } static gboolean -curl_timeout_cb (gpointer user_data) +_con_curl_timeout_cb (gpointer user_data) { gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data)); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); priv->concheck.curl_timer = 0; - curl_check_connectivity (priv->concheck.curl_mhandle, CURL_SOCKET_TIMEOUT, 0); + _con_curl_check_connectivity (priv->concheck.curl_mhandle, CURL_SOCKET_TIMEOUT, 0); return G_SOURCE_REMOVE; } @@ -307,12 +307,12 @@ multi_timer_cb (CURLM *multi, long timeout_ms, void *userdata) nm_clear_g_source (&priv->concheck.curl_timer); if (timeout_ms != -1) - priv->concheck.curl_timer = g_timeout_add (timeout_ms, curl_timeout_cb, self); + priv->concheck.curl_timer = g_timeout_add (timeout_ms, _con_curl_timeout_cb, self); return 0; } static gboolean -curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data) +_con_curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data) { gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data)); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); @@ -326,21 +326,21 @@ curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data) if (condition & G_IO_ERR) action |= CURL_CSELECT_ERR; - curl_check_connectivity (priv->concheck.curl_mhandle, fd, action); + _con_curl_check_connectivity (priv->concheck.curl_mhandle, fd, action); return G_SOURCE_CONTINUE; } typedef struct { GIOChannel *ch; guint ev; -} CurlSockData; +} ConCurlSockData; static int multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void *socketp) { NMConnectivity *self = NM_CONNECTIVITY (userdata); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); - CurlSockData *fdp = socketp; + ConCurlSockData *fdp = socketp; GIOCondition condition = 0; if (what == CURL_POLL_REMOVE) { @@ -348,11 +348,11 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void curl_multi_assign (priv->concheck.curl_mhandle, s, NULL); nm_clear_g_source (&fdp->ev); g_io_channel_unref (fdp->ch); - g_slice_free (CurlSockData, fdp); + g_slice_free (ConCurlSockData, fdp); } } else { if (!fdp) { - fdp = g_slice_new0 (CurlSockData); + fdp = g_slice_new0 (ConCurlSockData); fdp->ch = g_io_channel_unix_new (s); curl_multi_assign (priv->concheck.curl_mhandle, s, fdp); } else @@ -366,7 +366,7 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void condition = G_IO_IN | G_IO_OUT; if (condition) - fdp->ev = g_io_add_watch (fdp->ch, condition, curl_socketevent_cb, self); + fdp->ev = g_io_add_watch (fdp->ch, condition, _con_curl_socketevent_cb, self); } return CURLM_OK; From a54ffc6c016e594d4ec401db82d47e1520aa2e54 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 Jul 2018 15:34:10 +0200 Subject: [PATCH 2/4] connectivity/trivial: rename socket argument in multi_socket_cb() callback "s" might be a good name for a temporary string. But here it's really a file descriptor. Call it "fd". (cherry picked from commit cd0bd8a2eeee42ffbbe32cf83b306f6e8ebb1d82) --- src/nm-connectivity.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index fe0c594f02..fdcca49346 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -336,7 +336,7 @@ typedef struct { } ConCurlSockData; static int -multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void *socketp) +multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, void *socketp) { NMConnectivity *self = NM_CONNECTIVITY (userdata); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); @@ -345,7 +345,7 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void if (what == CURL_POLL_REMOVE) { if (fdp) { - curl_multi_assign (priv->concheck.curl_mhandle, s, NULL); + curl_multi_assign (priv->concheck.curl_mhandle, fd, NULL); nm_clear_g_source (&fdp->ev); g_io_channel_unref (fdp->ch); g_slice_free (ConCurlSockData, fdp); @@ -353,8 +353,8 @@ multi_socket_cb (CURL *e_handle, curl_socket_t s, int what, void *userdata, void } else { if (!fdp) { fdp = g_slice_new0 (ConCurlSockData); - fdp->ch = g_io_channel_unix_new (s); - curl_multi_assign (priv->concheck.curl_mhandle, s, fdp); + fdp->ch = g_io_channel_unix_new (fd); + curl_multi_assign (priv->concheck.curl_mhandle, fd, fdp); } else nm_clear_g_source (&fdp->ev); From 4af9ae13e045fc4f12a757eda99b3da85f7b6a50 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 Jul 2018 15:36:19 +0200 Subject: [PATCH 3/4] connectivity: add compile time check that "curl_socket_t" is a typedef to plain "int" 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. (cherry picked from commit 970af59731dbb0098b51f9302ce061b2ad67f25a) --- src/nm-connectivity.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index fdcca49346..3d45000b81 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -343,6 +343,8 @@ multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, voi ConCurlSockData *fdp = socketp; GIOCondition condition = 0; + (void) _NM_ENSURE_TYPE (int, fd); + if (what == CURL_POLL_REMOVE) { if (fdp) { curl_multi_assign (priv->concheck.curl_mhandle, fd, NULL); From 660e308dd28fb502e2f9e58044684c2452cf6f6e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 24 Jul 2018 15:24:50 +0200 Subject: [PATCH 4/4] connectivity: avoid busy looping with connectivity-check failed 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 (cherry picked from commit 884a28b28ce1ae61c2cab92ff6205e5840c628fa) --- src/nm-connectivity.c | 61 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 3d45000b81..1ce6d3cfeb 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -234,7 +234,7 @@ _check_handle_get_response (NMConnectivityCheckHandle *cb_data) return cb_data->concheck.response ?: NM_CONFIG_DEFAULT_CONNECTIVITY_RESPONSE; } -static void +static gboolean _con_curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) { NMConnectivityCheckHandle *cb_data; @@ -244,10 +244,13 @@ _con_curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) long response_code; CURLMcode ret; int running_handles; + gboolean success = TRUE; ret = curl_multi_socket_action (mhandle, sockfd, ev_bitmask, &running_handles); - if (ret != CURLM_OK) + if (ret != CURLM_OK) { _LOGD ("connectivity check failed: %d", ret); + success = FALSE; + } while ((msg = curl_multi_info_read (mhandle, &m_left))) { @@ -258,6 +261,7 @@ _con_curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) eret = curl_easy_getinfo (msg->easy_handle, CURLINFO_PRIVATE, (char **) &cb_data); if (eret != CURLE_OK) { _LOGD ("curl cannot extract cb_data for easy handle, skipping msg"); + success = FALSE; continue; } @@ -286,6 +290,12 @@ _con_curl_check_connectivity (CURLM *mhandle, int sockfd, int ev_bitmask) "unexpected short response"); } } + + /* if we return a failure, we don't know what went wrong. It's likely serious, because + * a failure here is not expected. Return FALSE, so that we stop polling the file descriptor. + * Worst case, this leaves the pending connectivity check unhandled, until our regular + * time-out kicks in. */ + return success; } static gboolean @@ -311,13 +321,30 @@ multi_timer_cb (CURLM *multi, long timeout_ms, void *userdata) return 0; } +typedef struct { + NMConnectivity *self; + GIOChannel *ch; + + /* this is a very simplistic weak-pointer. If ConCurlSockData gets + * destroyed, it will set *destroy_notify to TRUE. + * + * _con_curl_socketevent_cb() uses this to detect whether it can + * safely access @fdp after _con_curl_check_connectivity(). */ + gboolean *destroy_notify; + + guint ev; +} ConCurlSockData; + static gboolean _con_curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_data) { - gs_unref_object NMConnectivity *self = g_object_ref (NM_CONNECTIVITY (user_data)); + ConCurlSockData *fdp = user_data; + gs_unref_object NMConnectivity *self = g_object_ref (fdp->self); NMConnectivityPrivate *priv = NM_CONNECTIVITY_GET_PRIVATE (self); int fd = g_io_channel_unix_get_fd (ch); int action = 0; + gboolean fdp_destroyed = FALSE; + gboolean success; if (condition & G_IO_IN) action |= CURL_CSELECT_IN; @@ -326,14 +353,23 @@ _con_curl_socketevent_cb (GIOChannel *ch, GIOCondition condition, gpointer user_ if (condition & G_IO_ERR) action |= CURL_CSELECT_ERR; - _con_curl_check_connectivity (priv->concheck.curl_mhandle, fd, action); - return G_SOURCE_CONTINUE; -} + nm_assert (!fdp->destroy_notify); + fdp->destroy_notify = &fdp_destroyed; -typedef struct { - GIOChannel *ch; - guint ev; -} ConCurlSockData; + success = _con_curl_check_connectivity (priv->concheck.curl_mhandle, fd, action); + + if (fdp_destroyed) { + /* hups. fdp got invalidated during _con_curl_check_connectivity(). That's fine, + * just don't touch it. */ + } else { + nm_assert (fdp->destroy_notify == &fdp_destroyed); + fdp->destroy_notify = NULL; + if (!success) + fdp->ev = 0; + } + + return success ? G_SOURCE_CONTINUE : G_SOURCE_REMOVE; +} static int multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, void *socketp) @@ -347,6 +383,8 @@ multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, voi if (what == CURL_POLL_REMOVE) { if (fdp) { + if (fdp->destroy_notify) + *fdp->destroy_notify = TRUE; curl_multi_assign (priv->concheck.curl_mhandle, fd, NULL); nm_clear_g_source (&fdp->ev); g_io_channel_unref (fdp->ch); @@ -355,6 +393,7 @@ multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, voi } else { if (!fdp) { fdp = g_slice_new0 (ConCurlSockData); + fdp->self = self; fdp->ch = g_io_channel_unix_new (fd); curl_multi_assign (priv->concheck.curl_mhandle, fd, fdp); } else @@ -368,7 +407,7 @@ multi_socket_cb (CURL *e_handle, curl_socket_t fd, int what, void *userdata, voi condition = G_IO_IN | G_IO_OUT; if (condition) - fdp->ev = g_io_add_watch (fdp->ch, condition, _con_curl_socketevent_cb, self); + fdp->ev = g_io_add_watch (fdp->ch, condition, _con_curl_socketevent_cb, fdp); } return CURLM_OK;