From 7fe8aa68a21465b7e33923dbc68d41f01b4c88f9 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 10 Sep 2018 11:58:08 +0100 Subject: [PATCH 1/4] gnetworkmonitornetlink: Don't check if a passed-in GError ** is NULL This is not a correct way to check if `g_socket_new_from_fd()` failed. Instead just see if it returned `NULL` itself. This was preventing the netlink monitor from being initialised. Closes #1518 --- gio/gnetworkmonitornetlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gio/gnetworkmonitornetlink.c b/gio/gnetworkmonitornetlink.c index b308b3b65..f889a3b64 100644 --- a/gio/gnetworkmonitornetlink.c +++ b/gio/gnetworkmonitornetlink.c @@ -113,7 +113,7 @@ g_network_monitor_netlink_initable_init (GInitable *initable, } nl->priv->sock = g_socket_new_from_fd (sockfd, error); - if (error) + if (!nl->priv->sock) { g_prefix_error (error, "%s", _("Could not create network monitor: ")); (void) g_close (sockfd, NULL); From 6629423e7a41e3f177feb74dc59fd6b460aceed2 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 10 Sep 2018 12:00:46 +0100 Subject: [PATCH 2/4] gnetworkmonitornetlink: Close the socket after disconnecting its GSources `read_netlink_messages()` is the callback attached to the netlink socket (G_IO_IN). It calls `g_socket_receive_message()`. There is a race condition that if the socket is closed while there is a pending call, we will try to receive on a closed socket, which fails. To avoid this, we switch the order of the operations around: first destroy the source and then close the socket. --- gio/gnetworkmonitornetlink.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/gio/gnetworkmonitornetlink.c b/gio/gnetworkmonitornetlink.c index f889a3b64..3841e69e1 100644 --- a/gio/gnetworkmonitornetlink.c +++ b/gio/gnetworkmonitornetlink.c @@ -435,12 +435,6 @@ g_network_monitor_netlink_finalize (GObject *object) { GNetworkMonitorNetlink *nl = G_NETWORK_MONITOR_NETLINK (object); - if (nl->priv->sock) - { - g_socket_close (nl->priv->sock, NULL); - g_object_unref (nl->priv->sock); - } - if (nl->priv->source) { g_source_destroy (nl->priv->source); @@ -453,6 +447,12 @@ g_network_monitor_netlink_finalize (GObject *object) g_source_unref (nl->priv->dump_source); } + if (nl->priv->sock) + { + g_socket_close (nl->priv->sock, NULL); + g_object_unref (nl->priv->sock); + } + g_clear_pointer (&nl->priv->context, g_main_context_unref); g_clear_pointer (&nl->priv->dump_networks, g_ptr_array_unref); From 36c79adb2fdf4eadb7a96fcb753deecb1a66eb9b Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 10 Sep 2018 12:07:48 +0100 Subject: [PATCH 3/4] gnetworkmonitornetlink: Pass a GError into read_netlink_messages() Currently this function calls `g_warning()` explicitly. It would be nicer to properly propagate these failure up to the caller that tried to initialise us. --- gio/gnetworkmonitornetlink.c | 78 +++++++++++++++++++++++++----------- 1 file changed, 55 insertions(+), 23 deletions(-) diff --git a/gio/gnetworkmonitornetlink.c b/gio/gnetworkmonitornetlink.c index 3841e69e1..95de87929 100644 --- a/gio/gnetworkmonitornetlink.c +++ b/gio/gnetworkmonitornetlink.c @@ -52,9 +52,11 @@ struct _GNetworkMonitorNetlinkPrivate GPtrArray *dump_networks; }; -static gboolean read_netlink_messages (GSocket *socket, - GIOCondition condition, - gpointer user_data); +static gboolean read_netlink_messages (GNetworkMonitorNetlink *nl, + GError **error); +static gboolean read_netlink_messages_callback (GSocket *socket, + GIOCondition condition, + gpointer user_data); static gboolean request_dump (GNetworkMonitorNetlink *nl, GError **error); @@ -139,15 +141,20 @@ g_network_monitor_netlink_initable_init (GInitable *initable, */ while (nl->priv->dump_networks) { - if (!read_netlink_messages (NULL, G_IO_IN, nl)) - break; + GError *local_error = NULL; + if (!read_netlink_messages (nl, &local_error)) + { + g_warning ("%s", local_error->message); + g_clear_error (&local_error); + break; + } } g_socket_set_blocking (nl->priv->sock, FALSE); nl->priv->context = g_main_context_ref_thread_default (); nl->priv->source = g_socket_create_source (nl->priv->sock, G_IO_IN, NULL); g_source_set_callback (nl->priv->source, - (GSourceFunc) read_netlink_messages, nl, NULL); + (GSourceFunc) read_netlink_messages_callback, nl, NULL); g_source_attach (nl->priv->source, nl->priv->context); return initable_parent_iface->init (initable, cancellable, error); @@ -287,15 +294,13 @@ finish_dump (GNetworkMonitorNetlink *nl) } static gboolean -read_netlink_messages (GSocket *socket, - GIOCondition condition, - gpointer user_data) +read_netlink_messages (GNetworkMonitorNetlink *nl, + GError **error) { - GNetworkMonitorNetlink *nl = user_data; GInputVector iv; gssize len; gint flags; - GError *error = NULL; + GError *local_error = NULL; GSocketAddress *addr = NULL; struct nlmsghdr *msg; struct rtmsg *rtmsg; @@ -310,11 +315,9 @@ read_netlink_messages (GSocket *socket, flags = MSG_PEEK | MSG_TRUNC; len = g_socket_receive_message (nl->priv->sock, NULL, &iv, 1, - NULL, NULL, &flags, NULL, &error); + NULL, NULL, &flags, NULL, &local_error); if (len < 0) { - g_warning ("Error on netlink socket: %s", error->message); - g_clear_error (&error); retval = FALSE; goto done; } @@ -322,19 +325,15 @@ read_netlink_messages (GSocket *socket, iv.buffer = g_malloc (len); iv.size = len; len = g_socket_receive_message (nl->priv->sock, &addr, &iv, 1, - NULL, NULL, NULL, NULL, &error); + NULL, NULL, NULL, NULL, &local_error); if (len < 0) { - g_warning ("Error on netlink socket: %s", error->message); - g_clear_error (&error); retval = FALSE; goto done; } - if (!g_socket_address_to_native (addr, &source_sockaddr, sizeof (source_sockaddr), &error)) + if (!g_socket_address_to_native (addr, &source_sockaddr, sizeof (source_sockaddr), &local_error)) { - g_warning ("Error on netlink socket: %s", error->message); - g_clear_error (&error); retval = FALSE; goto done; } @@ -348,7 +347,10 @@ read_netlink_messages (GSocket *socket, { if (!NLMSG_OK (msg, (size_t) len)) { - g_warning ("netlink message was truncated; shouldn't happen..."); + g_set_error_literal (&local_error, + G_IO_ERROR, + G_IO_ERROR_PARTIAL_INPUT, + "netlink message was truncated; shouldn't happen..."); retval = FALSE; goto done; } @@ -409,13 +411,21 @@ read_netlink_messages (GSocket *socket, { struct nlmsgerr *e = NLMSG_DATA (msg); - g_warning ("netlink error: %s", g_strerror (-e->error)); + g_set_error (&local_error, + G_IO_ERROR, + g_io_error_from_errno (-e->error), + "netlink error: %s", + g_strerror (-e->error)); } retval = FALSE; goto done; default: - g_warning ("unexpected netlink message %d", msg->nlmsg_type); + g_set_error (&local_error, + G_IO_ERROR, + G_IO_ERROR_INVALID_DATA, + "unexpected netlink message %d", + msg->nlmsg_type); retval = FALSE; goto done; } @@ -427,6 +437,10 @@ read_netlink_messages (GSocket *socket, if (!retval && nl->priv->dump_networks) finish_dump (nl); + + if (local_error) + g_propagate_prefixed_error (error, local_error, "Error on netlink socket: "); + return retval; } @@ -459,6 +473,24 @@ g_network_monitor_netlink_finalize (GObject *object) G_OBJECT_CLASS (g_network_monitor_netlink_parent_class)->finalize (object); } +static gboolean +read_netlink_messages_callback (GSocket *socket, + GIOCondition condition, + gpointer user_data) +{ + GError *error = NULL; + GNetworkMonitorNetlink *nl = G_NETWORK_MONITOR_NETLINK (user_data); + + if (!read_netlink_messages (nl, &error)) + { + g_warning ("Error reading netlink message: %s", error->message); + g_clear_error (&error); + return FALSE; + } + + return TRUE; +} + static void g_network_monitor_netlink_class_init (GNetworkMonitorNetlinkClass *nl_class) { From 6eb5bd8192113b1a50e06dd732270c98c4edddee Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 10 Sep 2018 12:09:50 +0100 Subject: [PATCH 4/4] network-monitor-race test: Have the subprocess inherit stdout and stderr If initialising the monitors produces any debug output, it is good to be able to see it when running this test. --- gio/tests/network-monitor-race.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gio/tests/network-monitor-race.c b/gio/tests/network-monitor-race.c index cadd62cee..9c6866e6a 100644 --- a/gio/tests/network-monitor-race.c +++ b/gio/tests/network-monitor-race.c @@ -74,7 +74,10 @@ test_network_monitor (void) for (ii = 0; ii < MAX_RUNS; ii++) { - g_test_trap_subprocess (NULL, 0, 0); + g_test_trap_subprocess (NULL, + 0, + G_TEST_SUBPROCESS_INHERIT_STDOUT | + G_TEST_SUBPROCESS_INHERIT_STDERR); g_test_trap_assert_passed (); } }