extcap: Close capture session after extcap finishes

Wait up to 30 seconds for extcap process to finish after closing pipes. The wait is achieved in non-blocking fashion, i.e. the UI is completely responsive during the wait. Only actions related to capture process like capture control, file open, save, export are inactive during the wait. On Windows extcap child watch callback gets called immediately as the process is forcefully terminated. Prior to this change the extcap was forcefully terminated on Windows anyway. The wait is possible on UNIX systems if extcap does handle SIGPIPE and SIGTERM signals. The defaults handlers for SIGPIPE and SIGTERM simply terminate the process so for large number of extcaps there is no change. If extcap does not finish within 30 seconds, it is forcefully terminated using SIGKILL signal.
2022-08-07 12:31:48 +02:00 · 2022-08-07 12:31:48 +02:00 · c1861ad1cc
parent 86c6509cf3
commit c1861ad1cc
9 changed files with 177 additions and 139 deletions
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@ -2457,7 +2457,6 @@ set_target_properties(copy_data_files PROPERTIES FOLDER "Copy Tasks")
 # sources common for wireshark, tshark, rawshark and sharkd
 add_library(shark_common OBJECT
 	cfile.c
-	extcap.c
 	extcap_parser.c
 	file_packet_provider.c
 	frame_tvbuff.c
@ -2476,6 +2475,7 @@ if(BUILD_wireshark AND QT_FOUND)
 	set(WIRESHARK_SRC
 		file.c
 		fileset.c
+		extcap.c
 		${PLATFORM_UI_SRC}
 	)
 	set(wireshark_FILES
@ -2492,6 +2492,7 @@ if(BUILD_logray AND QT_FOUND)
 	set(LOGRAY_SRC
 		file.c
 		fileset.c
+		extcap.c
 		${PLATFORM_UI_SRC}
 	)
 	set(logray_FILES
@ -2852,6 +2853,7 @@ if(BUILD_tshark)
 		$<TARGET_OBJECTS:shark_common>
 		tshark-tap-register.c
 		tshark.c
+		extcap.c
 		${TSHARK_TAP_SRC}
 	)

--- a/capture/capture_session.h
+++ b/capture/capture_session.h
@ -124,6 +124,8 @@ capture_session_init(capture_session *cap_session, capture_file *cf,
                     new_file_fn new_file, new_packets_fn new_packets,
                     drops_fn drops, error_fn error,
                     cfilter_error_fn cfilter_error, closed_fn closed);
+
+void capture_process_finished(capture_session *cap_session);
 #else

 /* dummy is needed because clang throws the error: empty struct has size 0 in C, size 1 in C++ */
--- a/capture/capture_sync.c
+++ b/capture/capture_sync.c
@ -147,6 +147,56 @@ capture_session_init(capture_session *cap_session, capture_file *cf,
    cap_session->closed                          = closed;
 }

+void capture_process_finished(capture_session *cap_session)
+{
+    capture_options *capture_opts = cap_session->capture_opts;
+    interface_options *interface_opts;
+    GString *message;
+    guint i;
+
+    if (cap_session->fork_child != WS_INVALID_PID) {
+        /* Child process still running, session is not closed yet */
+        return;
+    }
+
+    for (i = 0; i < capture_opts->ifaces->len; i++) {
+        interface_opts = &g_array_index(capture_opts->ifaces, interface_options, i);
+        if ((interface_opts->if_type == IF_EXTCAP) &&
+            (interface_opts->extcap_pid != WS_INVALID_PID)) {
+            /* Atleast one extcap process did not finish yet, wait for it */
+            return;
+        }
+    }
+
+    /* All child processes finished */
+    if (capture_opts->extcap_terminate_id > 0) {
+        g_source_remove(capture_opts->extcap_terminate_id);
+        capture_opts->extcap_terminate_id = 0;
+    }
+
+    /* Construct message and close session */
+    message = g_string_new(capture_opts->closed_msg);
+    for (i = 0; i < capture_opts->ifaces->len; i++) {
+        interface_opts = &g_array_index(capture_opts->ifaces, interface_options, i);
+        if (interface_opts->if_type != IF_EXTCAP) {
+            continue;
+        }
+
+        if (interface_opts->extcap_stderr != NULL) {
+            if (message->len > 0) {
+                g_string_append(message, "\n");
+            }
+            g_string_append(message, "Error by extcap pipe: ");
+            g_string_append(message, interface_opts->extcap_stderr);
+        }
+    }
+
+    cap_session->closed(cap_session, message->str);
+    g_string_free(message, TRUE);
+    g_free(capture_opts->closed_msg);
+    capture_opts->closed_msg = NULL;
+}
+
 /* Append an arg (realloc) to an argc/argv array */
 /* (add a string pointer to a NULL-terminated array of string pointers) */
 static char **
@ -303,8 +353,9 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
    capture_opts_log(LOG_DOMAIN_CAPTURE, LOG_LEVEL_DEBUG, capture_opts);

    cap_session->fork_child = WS_INVALID_PID;
+    cap_session->capture_opts = capture_opts;

-    if (!extcap_init_interfaces(capture_opts)) {
+    if (!extcap_init_interfaces(cap_session)) {
        report_failure("Unable to init extcaps. (tmp fifo already exists?)");
        return FALSE;
    }
@ -717,7 +768,6 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
    }

    cap_session->fork_child_status = 0;
-    cap_session->capture_opts = capture_opts;
    cap_session->cap_data_info = cap_data;

    /* we might wait for a moment till child is ready, so update screen now */
@ -1763,9 +1813,9 @@ sync_pipe_input_cb(gint source, gpointer user_data)
        ws_close(cap_session->signal_pipe_write_fd);
 #endif
        ws_debug("cleaning extcap pipe");
-        extcap_if_cleanup(cap_session->capture_opts, &primary_msg);
-        cap_session->closed(cap_session, primary_msg);
-        g_free(primary_msg);
+        extcap_if_cleanup(cap_session);
+        cap_session->capture_opts->closed_msg = primary_msg;
+        capture_process_finished(cap_session);
        return FALSE;
    }

--- a/capture_opts.c
+++ b/capture_opts.c
@ -63,8 +63,10 @@ capture_opts_init(capture_options *capture_opts)
    capture_opts->default_options.extcap          = NULL;
    capture_opts->default_options.extcap_fifo     = NULL;
    capture_opts->default_options.extcap_args     = NULL;
-    capture_opts->default_options.extcap_pipedata = NULL;
    capture_opts->default_options.extcap_pid      = WS_INVALID_PID;
+    capture_opts->default_options.extcap_pipedata = NULL;
+    capture_opts->default_options.extcap_stderr   = NULL;
+    capture_opts->default_options.extcap_child_watch = 0;
 #ifdef _WIN32
    capture_opts->default_options.extcap_pipe_h   = INVALID_HANDLE_VALUE;
    capture_opts->default_options.extcap_control_in_h  = INVALID_HANDLE_VALUE;
@ -129,6 +131,8 @@ capture_opts_init(capture_options *capture_opts)
    capture_opts->print_name_to                   = NULL;
    capture_opts->temp_dir                        = NULL;
    capture_opts->compress_type                   = NULL;
+    capture_opts->closed_msg                      = NULL;
+    capture_opts->extcap_terminate_id             = 0;
 }

 void
@ -155,6 +159,15 @@ capture_opts_cleanup(capture_options *capture_opts)
    }
    g_free(capture_opts->save_file);
    g_free(capture_opts->temp_dir);
+
+    if (capture_opts->closed_msg) {
+        g_free(capture_opts->closed_msg);
+        capture_opts->closed_msg = NULL;
+    }
+    if (capture_opts->extcap_terminate_id > 0) {
+        g_source_remove(capture_opts->extcap_terminate_id);
+        capture_opts->extcap_terminate_id = 0;
+    }
 }

 /* log content of capture_opts */
@ -773,6 +786,8 @@ capture_opts_add_iface_opt(capture_options *capture_opts, const char *optarg_str
    interface_opts.extcap_args = NULL;
    interface_opts.extcap_pid = WS_INVALID_PID;
    interface_opts.extcap_pipedata = NULL;
+    interface_opts.extcap_stderr = NULL;
+    interface_opts.extcap_child_watch = 0;
 #ifdef _WIN32
    interface_opts.extcap_pipe_h = INVALID_HANDLE_VALUE;
    interface_opts.extcap_control_in_h = INVALID_HANDLE_VALUE;
@ -1274,6 +1289,7 @@ capture_opts_del_iface(capture_options *capture_opts, guint if_index)
    if (interface_opts->extcap_pid != WS_INVALID_PID)
        ws_pipe_close((ws_pipe_t *) interface_opts->extcap_pipedata);
    g_free(interface_opts->extcap_pipedata);
+    g_free(interface_opts->extcap_stderr);
    g_free(interface_opts->extcap_control_in);
    g_free(interface_opts->extcap_control_out);
 #ifdef HAVE_PCAP_REMOTE
@ -1328,6 +1344,7 @@ collect_ifaces(capture_options *capture_opts)
            if (interface_opts.extcap_args)
                g_hash_table_ref(interface_opts.extcap_args);
            interface_opts.extcap_pipedata = NULL;
+            interface_opts.extcap_stderr = NULL;
 #ifdef _WIN32
            interface_opts.extcap_pipe_h = INVALID_HANDLE_VALUE;
            interface_opts.extcap_control_in_h = INVALID_HANDLE_VALUE;
--- a/capture_opts.h
+++ b/capture_opts.h
@ -210,6 +210,7 @@ typedef struct interface_options_tag {
    GHashTable       *extcap_args;
    GPid              extcap_pid;           /* pid of running process or WS_INVALID_PID */
    gpointer          extcap_pipedata;
+    gchar            *extcap_stderr;
    guint             extcap_child_watch;
 #ifdef _WIN32
    HANDLE            extcap_pipe_h;
@ -326,6 +327,8 @@ typedef struct capture_options_tag {
    gboolean           output_to_pipe;        /**< save_file is a pipe (named or stdout) */
    gboolean           capture_child;         /**< hidden option: Wireshark child mode */
    gchar             *compress_type;         /**< compress type */
+    gchar             *closed_msg;            /**< Dumpcap capture closed message */
+    guint              extcap_terminate_id;   /**< extcap process termination source ID */
 } capture_options;

 /* initialize the capture_options with some reasonable values */
--- a/extcap.c
+++ b/extcap.c
@ -44,6 +44,7 @@
 #include <wsutil/wslog.h>
 #include <wsutil/ws_assert.h>

+#include "capture/capture_session.h"
 #include "capture_opts.h"

 #include "extcap.h"
@ -51,7 +52,10 @@

 #include "ui/version_info.h"

-static void extcap_child_watch_cb(GPid pid, gint status, gpointer user_data);
+/* Number of seconds to wait for extcap process to exit after cleanup.
+ * If extcap does not exit before the timeout, it is forcefully terminated.
+ */
+#define EXTCAP_CLEANUP_TIMEOUT 30

 /* internal container, for all the extcap executables that have been found.
 * Will be reset if extcap_clear_interfaces() is being explicitly called
@ -1157,14 +1161,13 @@ extcap_has_toolbar(const char *ifname)
    return FALSE;
 }

-void extcap_if_cleanup(capture_options *capture_opts, gchar **errormsg)
+#ifdef HAVE_LIBPCAP
+static gboolean extcap_terminate_cb(gpointer user_data)
 {
+    capture_session *cap_session = (capture_session *)user_data;
+    capture_options *capture_opts = cap_session->capture_opts;
    interface_options *interface_opts;
-    ws_pipe_t *pipedata;
-    guint icnt = 0;
-    gboolean overwrite_exitcode;
-    gchar *buffer;
-#define STDERR_BUFFER_SIZE 1024
+    guint icnt;

    for (icnt = 0; icnt < capture_opts->ifaces->len; icnt++)
    {
@ -1177,7 +1180,37 @@ void extcap_if_cleanup(capture_options *capture_opts, gchar **errormsg)
            continue;
        }

-        overwrite_exitcode = FALSE;
+        if (interface_opts->extcap_pid != WS_INVALID_PID)
+        {
+#ifdef _WIN32
+            /* extcap_if_cleanup() already called TerminateProcess() */
+#else
+            kill(interface_opts->extcap_pid, SIGKILL);
+#endif
+        }
+    }
+
+    capture_opts->extcap_terminate_id = 0;
+    return G_SOURCE_REMOVE;
+}
+
+void extcap_if_cleanup(capture_session *cap_session)
+{
+    capture_options *capture_opts = cap_session->capture_opts;
+    interface_options *interface_opts;
+    guint icnt = 0;
+    gboolean extcaps_alive = FALSE;
+
+    for (icnt = 0; icnt < capture_opts->ifaces->len; icnt++)
+    {
+        interface_opts = &g_array_index(capture_opts->ifaces, interface_options,
+                                       icnt);
+
+        /* skip native interfaces */
+        if (interface_opts->if_type != IF_EXTCAP)
+        {
+            continue;
+        }

        ws_debug("Extcap [%s] - Cleaning up fifo: %s; PID: %d", interface_opts->name,
              interface_opts->extcap_fifo, interface_opts->extcap_pid);
@ -1223,98 +1256,26 @@ void extcap_if_cleanup(capture_options *capture_opts, gchar **errormsg)
            ws_unlink(interface_opts->extcap_control_out);
            interface_opts->extcap_control_out = NULL;
        }
+#endif
        /* Send termination signal to child. On Linux and OSX the child will not notice that the
         * pipe has been closed before writing to the pipe.
         */
        if (interface_opts->extcap_pid != WS_INVALID_PID)
        {
+            extcaps_alive = TRUE;
+#ifdef _WIN32
+            /* Not nice, but Wireshark has been doing so for years */
+            TerminateProcess(interface_opts->extcap_pid, 0);
+#else
            kill(interface_opts->extcap_pid, SIGTERM);
-        }
 #endif
-        /* Maybe the client closed and removed fifo, but ws should check if
-         * pid should be closed */
-        ws_debug("Extcap [%s] - Closing spawned PID: %d", interface_opts->name,
-              interface_opts->extcap_pid);
-
-        pipedata = (ws_pipe_t *) interface_opts->extcap_pipedata;
-        if (pipedata)
-        {
-            if (pipedata->stderr_fd > 0)
-            {
-                buffer = (gchar *)g_malloc0(STDERR_BUFFER_SIZE + 1);
-                ws_read_string_from_pipe(ws_get_pipe_handle(pipedata->stderr_fd), buffer, STDERR_BUFFER_SIZE + 1);
-                if (strlen(buffer) > 0)
-                {
-                    pipedata->stderr_msg = g_strdup(buffer);
-                    pipedata->exitcode = 1;
-                }
-                g_free(buffer);
-            }
-
-#ifndef _WIN32
-            /* Final child watch may not have been called */
-            if (interface_opts->extcap_child_watch > 0)
-            {
-                extcap_child_watch_cb(pipedata->pid, 0, capture_opts);
-                /* it will have changed in extcap_child_watch_cb */
-                interface_opts = &g_array_index(capture_opts->ifaces, interface_options,
-                                               icnt);
-            }
-#endif
-
-            if (pipedata->stderr_msg != NULL)
-            {
-                overwrite_exitcode = TRUE;
-            }
-
-            if (overwrite_exitcode || pipedata->exitcode != 0)
-            {
-                if (pipedata->stderr_msg != NULL)
-                {
-                    if (*errormsg == NULL)
-                    {
-                        *errormsg = ws_strdup_printf("Error by extcap pipe: %s", pipedata->stderr_msg);
-                    }
-                    else
-                    {
-                        gchar *temp = g_strconcat(*errormsg, "\nError by extcap pipe: " , pipedata->stderr_msg, NULL);
-                        g_free(*errormsg);
-                        *errormsg = temp;
-                    }
-                    g_free(pipedata->stderr_msg);
-                }
-
-                pipedata->stderr_msg = NULL;
-                pipedata->exitcode = 0;
-            }
        }
+    }

-        if (interface_opts->extcap_child_watch > 0)
-        {
-            g_source_remove(interface_opts->extcap_child_watch);
-            interface_opts->extcap_child_watch = 0;
-        }
-
-        if (pipedata) {
-            if (pipedata->stdout_fd > 0)
-            {
-                ws_close(pipedata->stdout_fd);
-            }
-
-            if (pipedata->stderr_fd > 0)
-            {
-                ws_close(pipedata->stderr_fd);
-            }
-
-            if (interface_opts->extcap_pid != WS_INVALID_PID)
-            {
-                ws_pipe_close(pipedata);
-                interface_opts->extcap_pid = WS_INVALID_PID;
-
-                g_free(pipedata);
-                interface_opts->extcap_pipedata = NULL;
-            }
-        }
+    if (extcaps_alive)
+    {
+        capture_opts->extcap_terminate_id =
+            g_timeout_add_seconds(EXTCAP_CLEANUP_TIMEOUT, extcap_terminate_cb, cap_session);
    }
 }

@ -1338,17 +1299,13 @@ extcap_add_arg_and_remove_cb(gpointer key, gpointer value, gpointer data)
    return FALSE;
 }

-void extcap_child_watch_cb(GPid pid, gint status, gpointer user_data)
+static void extcap_child_watch_cb(GPid pid, gint status _U_, gpointer user_data)
 {
    guint i;
    interface_options *interface_opts;
    ws_pipe_t *pipedata = NULL;
-    capture_options *capture_opts = (capture_options *)(user_data);
-
-    if (capture_opts == NULL || capture_opts->ifaces == NULL || capture_opts->ifaces->len == 0)
-    {
-        return;
-    }
+    capture_session *cap_session = (capture_session *)(user_data);
+    capture_options *capture_opts = cap_session->capture_opts;

    /* Close handle to child process. */
    g_spawn_close_pid(pid);
@ -1359,36 +1316,39 @@ void extcap_child_watch_cb(GPid pid, gint status, gpointer user_data)
        interface_opts = &g_array_index(capture_opts->ifaces, interface_options, i);
        if (interface_opts->extcap_pid == pid)
        {
+            ws_debug("Extcap [%s] - Closing spawned PID: %d", interface_opts->name,
+                     interface_opts->extcap_pid);
+            interface_opts->extcap_pid = WS_INVALID_PID;
+
            pipedata = (ws_pipe_t *)interface_opts->extcap_pipedata;
            if (pipedata != NULL)
            {
-                interface_opts->extcap_pid = WS_INVALID_PID;
-                pipedata->exitcode = 0;
-#ifndef _WIN32
-                if (WIFEXITED(status))
+                if (pipedata->stdout_fd > 0)
                {
-                    if (WEXITSTATUS(status) != 0)
+                    ws_close(pipedata->stdout_fd);
+                }
+
+                if (pipedata->stderr_fd > 0)
+                {
+#define STDERR_BUFFER_SIZE 1024
+                    gchar *buffer = (gchar *)g_malloc0(STDERR_BUFFER_SIZE + 1);
+                    ws_read_string_from_pipe(ws_get_pipe_handle(pipedata->stderr_fd), buffer, STDERR_BUFFER_SIZE + 1);
+                    if (strlen(buffer) > 0)
                    {
-                        pipedata->exitcode = WEXITSTATUS(status);
+                        interface_opts->extcap_stderr = g_strdup(buffer);
                    }
+                    g_free(buffer);
+                    ws_close(pipedata->stderr_fd);
                }
-                else
-                {
-                    pipedata->exitcode = G_SPAWN_ERROR_FAILED;
-                }
-#else
-                if (status != 0)
-                {
-                    pipedata->exitcode = status;
-                }
-#endif
-                if (status == 0 && pipedata->stderr_msg != NULL)
-                {
-                    pipedata->exitcode = 1;
-                }
+
+                g_free(pipedata);
+                interface_opts->extcap_pipedata = NULL;
            }
-            g_source_remove(interface_opts->extcap_child_watch);
+
            interface_opts->extcap_child_watch = 0;
+
+            /* Close session if this is the last remaining process */
+            capture_process_finished(cap_session);
            break;
        }
    }
@ -1584,8 +1544,9 @@ static gboolean extcap_create_pipe(const gchar *ifname, gchar **fifo, const gcha
 /* call mkfifo for each extcap,
 * returns FALSE if there's an error creating a FIFO */
 gboolean
-extcap_init_interfaces(capture_options *capture_opts)
+extcap_init_interfaces(capture_session *cap_session)
 {
+    capture_options *capture_opts = cap_session->capture_opts;
    guint i;
    interface_options *interface_opts;
    ws_pipe_t *pipedata;
@ -1657,7 +1618,8 @@ extcap_init_interfaces(capture_options *capture_opts)
        interface_opts->extcap_pid = pid;

        interface_opts->extcap_child_watch =
-            g_child_watch_add(pid, extcap_child_watch_cb, (gpointer)capture_opts);
+            g_child_watch_add_full(G_PRIORITY_HIGH, pid, extcap_child_watch_cb,
+                                   (gpointer)cap_session, NULL);

 #ifdef _WIN32
        /* On Windows, wait for extcap to connect to named pipe.
@ -1689,6 +1651,7 @@ extcap_init_interfaces(capture_options *capture_opts)

    return TRUE;
 }
+#endif /* HAVE_LIBPCAP */

 /************* EXTCAP LOAD INTERFACE LIST ***************
 *
--- a/extcap.h
+++ b/extcap.h
@ -22,6 +22,7 @@

 #include <wsutil/plugins.h>

+#include "capture/capture_session.h"
 #include <ui/capture_ui_utils.h>

 /* As boolean flags will be allowed any form of yes, true or any number != 0 (or starting with 0)
@ -205,22 +206,23 @@ extcap_has_configuration(const char * ifname, gboolean is_required);
 gboolean
 extcap_has_toolbar(const char *ifname);

+#ifdef HAVE_LIBPCAP
 /**
- * Initializes each extcap interface with the supplied capture options.
+ * Initializes each extcap interface with the supplied capture session.
 * Initializes the extcap interface list if that hasn't already been done.
- * @param capture_opts Capture options.
+ * @param cap_session Capture session.
 * @return TRUE on success, FALSE on failure.
 */
 gboolean
-extcap_init_interfaces(capture_options * capture_opts);
+extcap_init_interfaces(capture_session *cap_session);
+#endif /* HAVE_LIBPCAP */

 /**
 * Clean up all if related stuff.
- * @param capture_opts Capture options.
- * @param errormsg Set to NULL on success, error description on failure.
+ * @param cap_session Capture session.
 */
 void
-extcap_if_cleanup(capture_options * capture_opts, gchar ** errormsg);
+extcap_if_cleanup(capture_session *cap_session);

 /**
 * Fetch an extcap preference for a given argument.
--- a/rawshark.c
+++ b/rawshark.c
@ -52,6 +52,10 @@
 #include <wsutil/wslog.h>
 #include <ui/clopts_common.h>

+#ifdef _WIN32
+#include <wsutil/unicode-utils.h>
+#endif
+
 #include "globals.h"
 #include <epan/packet.h>
 #include <epan/ftypes/ftypes.h>
@ -84,8 +88,6 @@

 #include "capture/capture-pcap-util.h"

-#include "extcap.h"
-
 #ifdef HAVE_LIBPCAP
 #include <setjmp.h>
 #ifdef _WIN32
@ -812,7 +814,6 @@ clean_exit:
    g_free(pipe_name);
    epan_free(cfile.epan);
    epan_cleanup();
-    extcap_cleanup();
    wtap_cleanup();
    return ret;
 }
--- a/tfshark.c
+++ b/tfshark.c
@ -62,7 +62,6 @@
 #include <epan/tap.h>
 #include <epan/stat_tap_ui.h>
 #include <epan/ex-opt.h>
-#include "extcap.h"

 #include <wiretap/wtap-int.h>
 #include <wiretap/file_wrappers.h>
@ -966,7 +965,6 @@ clean_exit:
    destroy_print_stream(print_stream);
    epan_free(cfile.epan);
    epan_cleanup();
-    extcap_cleanup();

    output_fields_free(output_fields);
    output_fields = NULL;