From 64335f9ebf6041c172765f0be0aaf8c813099942 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 29 Oct 2014 14:39:51 +0100 Subject: [PATCH 1/3] dnsmasq: kill running process from pidfile synchronously This should avoid a race when stopping dnsmasq. This is still a hack, because we don't want to ever wait for a process to terminate. A better solution would be to have nm-dnsmasq-manager managing the global state. Not only the dnsmasq instance of on NMDevice. Instead it should keep track of all the child processes it starts and it wants to start. This way, starting a new process should be delayed until any (child or non-child) instances are gone. That however would be a bigger change. https://bugzilla.gnome.org/show_bug.cgi?id=728342 https://bugzilla.gnome.org/show_bug.cgi?id=762008 --- src/dnsmasq-manager/nm-dnsmasq-manager.c | 37 ++++++++++++++---------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/dnsmasq-manager/nm-dnsmasq-manager.c b/src/dnsmasq-manager/nm-dnsmasq-manager.c index f4a6208d05..6ebbcf5fb5 100644 --- a/src/dnsmasq-manager/nm-dnsmasq-manager.c +++ b/src/dnsmasq-manager/nm-dnsmasq-manager.c @@ -33,6 +33,7 @@ #include "nm-dnsmasq-utils.h" #include "nm-utils.h" #include "NetworkManagerUtils.h" +#include "nm-core-internal.h" typedef struct { char *iface; @@ -302,35 +303,41 @@ create_dm_cmd_line (const char *iface, } static void -kill_existing_for_iface (const char *iface, const char *pidfile) +kill_existing_by_pidfile (const char *pidfile) { char *contents = NULL; - glong pid; - char *proc_path = NULL; + pid_t pid; + char proc_path[250]; char *cmdline_contents = NULL; + guint64 start_time; + const char *exe; - if (!g_file_get_contents (pidfile, &contents, NULL, NULL)) + if ( !pidfile + || !g_file_get_contents (pidfile, &contents, NULL, NULL)) + return; + + pid = _nm_utils_ascii_str_to_int64 (contents, 10, 1, G_MAXUINT64, 0); + if (pid == 0) goto out; - pid = strtol (contents, NULL, 10); - if (pid < 1 || pid > INT_MAX) + start_time = nm_utils_get_start_time_for_pid (pid, NULL, NULL); + if (start_time == 0) goto out; - proc_path = g_strdup_printf ("/proc/%ld/cmdline", pid); + nm_sprintf_buf (proc_path, "/proc/%lld/cmdline", (long long) pid); if (!g_file_get_contents (proc_path, &cmdline_contents, NULL, NULL)) goto out; - if (strstr (cmdline_contents, "bin/dnsmasq")) { - if (kill (pid, 0) == 0) { - nm_log_dbg (LOGD_SHARING, "Killing stale dnsmasq process %ld", pid); - kill (pid, SIGKILL); - } - unlink (pidfile); + exe = strrchr (cmdline_contents, '/'); + if ( (exe && strcmp (&exe[1], "dnsmasq") == 0) + || (strcmp (cmdline_contents, DNSMASQ_PATH) == 0)) { + nm_utils_kill_process_sync (pid, start_time, SIGKILL, LOGD_SHARING, + "dnsmasq", 0, 0, 500); } out: + unlink (pidfile); g_free (cmdline_contents); - g_free (proc_path); g_free (contents); } @@ -349,7 +356,7 @@ nm_dnsmasq_manager_start (NMDnsMasqManager *manager, priv = NM_DNSMASQ_MANAGER_GET_PRIVATE (manager); - kill_existing_for_iface (priv->iface, priv->pidfile); + kill_existing_by_pidfile (priv->pidfile); dm_cmd = create_dm_cmd_line (priv->iface, ip4_config, priv->pidfile, error); if (!dm_cmd) From 1178f814a1deb4a85a4f6ad6542811aae8bc862d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 29 Oct 2014 14:03:09 +0100 Subject: [PATCH 2/3] dnsmasq: refactor creating dnsmasq command line args to pass listen_address argument --- src/dnsmasq-manager/nm-dnsmasq-manager.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/dnsmasq-manager/nm-dnsmasq-manager.c b/src/dnsmasq-manager/nm-dnsmasq-manager.c index 6ebbcf5fb5..41184910db 100644 --- a/src/dnsmasq-manager/nm-dnsmasq-manager.c +++ b/src/dnsmasq-manager/nm-dnsmasq-manager.c @@ -212,19 +212,20 @@ dm_watch_cb (GPid pid, gint status, gpointer user_data) static NMCmdLine * create_dm_cmd_line (const char *iface, - NMIP4Config *ip4_config, + const NMPlatformIP4Address *listen_address, const char *pidfile, GError **error) { NMCmdLine *cmd; GString *s; - const NMPlatformIP4Address *tmp; char first[INET_ADDRSTRLEN]; char last[INET_ADDRSTRLEN]; char localaddr[INET_ADDRSTRLEN]; char *error_desc = NULL; const char *dm_binary; + g_return_val_if_fail (listen_address, NULL); + dm_binary = nm_utils_find_helper ("dnsmasq", DNSMASQ_PATH, error); if (!dm_binary) return NULL; @@ -258,16 +259,13 @@ create_dm_cmd_line (const char *iface, */ nm_cmd_line_add_string (cmd, "--strict-order"); - /* Find the IP4 address to use */ - tmp = nm_ip4_config_get_address (ip4_config, 0); - s = g_string_new ("--listen-address="); - nm_utils_inet4_ntop (tmp->address, localaddr); + nm_utils_inet4_ntop (listen_address->address, localaddr); g_string_append (s, localaddr); nm_cmd_line_add_string (cmd, s->str); g_string_free (s, TRUE); - if (!nm_dnsmasq_utils_get_range (tmp, first, last, &error_desc)) { + if (!nm_dnsmasq_utils_get_range (listen_address, first, last, &error_desc)) { g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_FAILED, @@ -351,14 +349,14 @@ nm_dnsmasq_manager_start (NMDnsMasqManager *manager, gs_free char *cmd_str = NULL; g_return_val_if_fail (NM_IS_DNSMASQ_MANAGER (manager), FALSE); - if (error) - g_return_val_if_fail (*error == NULL, FALSE); + g_return_val_if_fail (!error || !*error, FALSE); + g_return_val_if_fail (nm_ip4_config_get_num_addresses (ip4_config) > 0, FALSE); priv = NM_DNSMASQ_MANAGER_GET_PRIVATE (manager); kill_existing_by_pidfile (priv->pidfile); - dm_cmd = create_dm_cmd_line (priv->iface, ip4_config, priv->pidfile, error); + dm_cmd = create_dm_cmd_line (priv->iface, nm_ip4_config_get_address (ip4_config, 0), priv->pidfile, error); if (!dm_cmd) return FALSE; From 0fbe055e2a4de8aa44d117500e5f3d508eb3af6c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 14 Feb 2016 10:40:00 +0100 Subject: [PATCH 3/3] utils: use stack allocated buffer for path in nm_utils_get_start_time_for_pid() --- src/NetworkManagerUtils.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index b59203255f..088811b5f2 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -482,7 +482,7 @@ guint64 nm_utils_get_start_time_for_pid (pid_t pid, char *out_state, pid_t *out_ppid) { guint64 start_time; - gs_free gchar *filename = NULL; + char filename[256]; gs_free gchar *contents = NULL; size_t length; gs_strfreev gchar **tokens = NULL; @@ -497,7 +497,7 @@ nm_utils_get_start_time_for_pid (pid_t pid, char *out_state, pid_t *out_ppid) g_return_val_if_fail (pid > 0, 0); - filename = g_strdup_printf ("/proc/%"G_GUINT64_FORMAT"/stat", (guint64) pid); + nm_sprintf_buf (filename, "/proc/%"G_GUINT64_FORMAT"/stat", (guint64) pid); if (!g_file_get_contents (filename, &contents, &length, NULL)) goto out;