From d0a9de2af1882f0ff23a43baa051da35d7fed512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mo=C5=84?= Date: Sat, 13 Aug 2022 05:58:50 +0200 Subject: [PATCH] capture: Stop extcaps before dumpcap Send SIGTERM on UNIX systems to all extcap processes when user requests capture stop. Wait up to 30 seconds for extcaps to finish. If extcaps do not finish in time, send SIGKILL to remaining extcaps. Do not call TerminateProcess() on Windows in the same place where UNIX SIGTERM is sent. Instead schedule extcap termination timeout to happen as soon as control returns back to the event loop. There is no universally agreed replacement for SIGTERM on Windows, so just keep things simple (forcefully terminate like always) until we have agreed on something. --- capture/capture_sync.c | 22 +++++-- capture_opts.c | 2 + capture_opts.h | 2 + extcap.c | 141 +++++++++++++++++++++++------------------ extcap.h | 5 +- ui/capture.c | 10 ++- 6 files changed, 109 insertions(+), 73 deletions(-) diff --git a/capture/capture_sync.c b/capture/capture_sync.c index e25dd9a385..a54f7596eb 100644 --- a/capture/capture_sync.c +++ b/capture/capture_sync.c @@ -154,13 +154,18 @@ void capture_process_finished(capture_session *cap_session) GString *message; guint i; - if (cap_session->fork_child != WS_INVALID_PID) { - /* Child process still running, session is not closed yet */ + if (!extcap_session_stop(cap_session)) { + /* Atleast one extcap process did not fully finish yet, wait for it */ return; } - if (!extcap_session_stop(cap_session)) { - /* Atleast one extcap process did not fully finish yet, wait for it */ + if (cap_session->fork_child != WS_INVALID_PID) { + if (capture_opts->stop_after_extcaps) { + /* User has requested capture stop and all extcaps are gone now */ + capture_opts->stop_after_extcaps = FALSE; + sync_pipe_stop(cap_session); + } + /* Wait for child process to end, session is not closed yet */ return; } @@ -186,6 +191,7 @@ void capture_process_finished(capture_session *cap_session) g_string_free(message, TRUE); g_free(capture_opts->closed_msg); capture_opts->closed_msg = NULL; + capture_opts->stop_after_extcaps = FALSE; } /* Append an arg (realloc) to an argc/argv array */ @@ -1803,10 +1809,12 @@ sync_pipe_input_cb(gint source, gpointer user_data) #ifdef _WIN32 ws_close(cap_session->signal_pipe_write_fd); #endif - ws_debug("cleaning extcap pipe"); - extcap_if_cleanup(cap_session); cap_session->capture_opts->closed_msg = primary_msg; - capture_process_finished(cap_session); + if (extcap_session_stop(cap_session)) { + capture_process_finished(cap_session); + } else { + extcap_request_stop(cap_session); + } return FALSE; } diff --git a/capture_opts.c b/capture_opts.c index f20509b684..53c1800bb7 100644 --- a/capture_opts.c +++ b/capture_opts.c @@ -128,6 +128,8 @@ capture_opts_init(capture_options *capture_opts) capture_opts->output_to_pipe = FALSE; capture_opts->capture_child = FALSE; + capture_opts->stop_after_extcaps = FALSE; + capture_opts->wait_for_extcap_cbs = FALSE; capture_opts->print_file_names = FALSE; capture_opts->print_name_to = NULL; capture_opts->temp_dir = NULL; diff --git a/capture_opts.h b/capture_opts.h index 130735b761..329b33c452 100644 --- a/capture_opts.h +++ b/capture_opts.h @@ -327,6 +327,8 @@ typedef struct capture_options_tag { /* internally used (don't touch from outside) */ gboolean output_to_pipe; /**< save_file is a pipe (named or stdout) */ gboolean capture_child; /**< hidden option: Wireshark child mode */ + gboolean stop_after_extcaps; /**< request dumpcap stop after last extcap */ + gboolean wait_for_extcap_cbs; /**< extcaps terminated, waiting for callbacks */ gchar *compress_type; /**< compress type */ gchar *closed_msg; /**< Dumpcap capture closed message */ guint extcap_terminate_id; /**< extcap process termination source ID */ diff --git a/extcap.c b/extcap.c index 62165fc9e5..9834c7dc2d 100644 --- a/extcap.c +++ b/extcap.c @@ -55,7 +55,12 @@ /* Number of seconds to wait for extcap process to exit after cleanup. * If extcap does not exit before the timeout, it is forcefully terminated. */ +#ifdef _WIN32 +/* Extcap interface does not specify SIGTERM replacement on Windows yet */ +#define EXTCAP_CLEANUP_TIMEOUT 0 +#else #define EXTCAP_CLEANUP_TIMEOUT 30 +#endif /* internal container, for all the extcap executables that have been found. * Will be reset if extcap_clear_interfaces() is being explicitly called @@ -1184,7 +1189,7 @@ static gboolean extcap_terminate_cb(gpointer user_data) if (interface_opts->extcap_pid != WS_INVALID_PID) { #ifdef _WIN32 - /* extcap_if_cleanup() already called TerminateProcess() */ + TerminateProcess(interface_opts->extcap_pid, 0); #else kill(interface_opts->extcap_pid, SIGKILL); #endif @@ -1205,20 +1210,39 @@ static gboolean extcap_terminate_cb(gpointer user_data) } } + capture_opts->wait_for_extcap_cbs = TRUE; capture_opts->extcap_terminate_id = 0; if (all_finished) { capture_process_finished(cap_session); } + return G_SOURCE_REMOVE; } -void extcap_if_cleanup(capture_session *cap_session) +void extcap_request_stop(capture_session *cap_session) { capture_options *capture_opts = cap_session->capture_opts; interface_options *interface_opts; guint icnt = 0; - gboolean extcaps_alive = FALSE; + + if (capture_opts->extcap_terminate_id > 0) + { + /* Already requested, do not extend timeout */ + return; + } + + if (capture_opts->wait_for_extcap_cbs) + { + /* Terminate callback was called, waiting for child callbacks */ + return; + } + + if (extcap_session_stop(cap_session)) + { + /* Nothing left to do, all extcaps have fully finished */ + return; + } for (icnt = 0; icnt < capture_opts->ifaces->len; icnt++) { @@ -1231,71 +1255,19 @@ void extcap_if_cleanup(capture_session *cap_session) continue; } - ws_debug("Extcap [%s] - Cleaning up fifo: %s; PID: %d", interface_opts->name, - interface_opts->extcap_fifo, interface_opts->extcap_pid); -#ifdef _WIN32 - if (interface_opts->extcap_pipe_h != INVALID_HANDLE_VALUE) - { - ws_debug("Extcap [%s] - Closing pipe", interface_opts->name); - FlushFileBuffers(interface_opts->extcap_pipe_h); - DisconnectNamedPipe(interface_opts->extcap_pipe_h); - CloseHandle(interface_opts->extcap_pipe_h); - interface_opts->extcap_pipe_h = INVALID_HANDLE_VALUE; - } - if (interface_opts->extcap_control_in_h != INVALID_HANDLE_VALUE) - { - ws_debug("Extcap [%s] - Closing control_in pipe", interface_opts->name); - FlushFileBuffers(interface_opts->extcap_control_in_h); - DisconnectNamedPipe(interface_opts->extcap_control_in_h); - CloseHandle(interface_opts->extcap_control_in_h); - interface_opts->extcap_control_in_h = INVALID_HANDLE_VALUE; - } - if (interface_opts->extcap_control_out_h != INVALID_HANDLE_VALUE) - { - ws_debug("Extcap [%s] - Closing control_out pipe", interface_opts->name); - FlushFileBuffers(interface_opts->extcap_control_out_h); - DisconnectNamedPipe(interface_opts->extcap_control_out_h); - CloseHandle(interface_opts->extcap_control_out_h); - interface_opts->extcap_control_out_h = INVALID_HANDLE_VALUE; - } -#else - if (interface_opts->extcap_fifo != NULL && file_exists(interface_opts->extcap_fifo)) - { - /* the fifo will not be freed here, but with the other capture_opts in capture_sync */ - ws_unlink(interface_opts->extcap_fifo); - interface_opts->extcap_fifo = NULL; - } - if (interface_opts->extcap_control_in && file_exists(interface_opts->extcap_control_in)) - { - ws_unlink(interface_opts->extcap_control_in); - interface_opts->extcap_control_in = NULL; - } - if (interface_opts->extcap_control_out && file_exists(interface_opts->extcap_control_out)) - { - 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. - */ + ws_debug("Extcap [%s] - Requesting stop PID: %d", interface_opts->name, + interface_opts->extcap_pid); + +#ifndef _WIN32 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 } +#endif } - if (extcaps_alive) - { - capture_opts->extcap_terminate_id = - g_timeout_add_seconds(EXTCAP_CLEANUP_TIMEOUT, extcap_terminate_cb, cap_session); - } + capture_opts->extcap_terminate_id = + g_timeout_add_seconds(EXTCAP_CLEANUP_TIMEOUT, extcap_terminate_cb, cap_session); } static gboolean @@ -1342,9 +1314,54 @@ gboolean extcap_session_stop(capture_session *cap_session) g_free(interface_opts->extcap_pipedata); interface_opts->extcap_pipedata = NULL; + +#ifdef _WIN32 + if (interface_opts->extcap_pipe_h != INVALID_HANDLE_VALUE) + { + ws_debug("Extcap [%s] - Closing pipe", interface_opts->name); + FlushFileBuffers(interface_opts->extcap_pipe_h); + DisconnectNamedPipe(interface_opts->extcap_pipe_h); + CloseHandle(interface_opts->extcap_pipe_h); + interface_opts->extcap_pipe_h = INVALID_HANDLE_VALUE; + } + if (interface_opts->extcap_control_in_h != INVALID_HANDLE_VALUE) + { + ws_debug("Extcap [%s] - Closing control_in pipe", interface_opts->name); + FlushFileBuffers(interface_opts->extcap_control_in_h); + DisconnectNamedPipe(interface_opts->extcap_control_in_h); + CloseHandle(interface_opts->extcap_control_in_h); + interface_opts->extcap_control_in_h = INVALID_HANDLE_VALUE; + } + if (interface_opts->extcap_control_out_h != INVALID_HANDLE_VALUE) + { + ws_debug("Extcap [%s] - Closing control_out pipe", interface_opts->name); + FlushFileBuffers(interface_opts->extcap_control_out_h); + DisconnectNamedPipe(interface_opts->extcap_control_out_h); + CloseHandle(interface_opts->extcap_control_out_h); + interface_opts->extcap_control_out_h = INVALID_HANDLE_VALUE; + } +#else + if (interface_opts->extcap_fifo != NULL && file_exists(interface_opts->extcap_fifo)) + { + /* the fifo will not be freed here, but with the other capture_opts in capture_sync */ + ws_unlink(interface_opts->extcap_fifo); + interface_opts->extcap_fifo = NULL; + } + if (interface_opts->extcap_control_in && file_exists(interface_opts->extcap_control_in)) + { + ws_unlink(interface_opts->extcap_control_in); + interface_opts->extcap_control_in = NULL; + } + if (interface_opts->extcap_control_out && file_exists(interface_opts->extcap_control_out)) + { + ws_unlink(interface_opts->extcap_control_out); + interface_opts->extcap_control_out = NULL; + } +#endif } /* All child processes finished */ + capture_opts->wait_for_extcap_cbs = FALSE; if (capture_opts->extcap_terminate_id > 0) { g_source_remove(capture_opts->extcap_terminate_id); diff --git a/extcap.h b/extcap.h index 400757557d..400d2149f9 100644 --- a/extcap.h +++ b/extcap.h @@ -226,11 +226,12 @@ extcap_init_interfaces(capture_session *cap_session); #endif /* HAVE_LIBPCAP */ /** - * Clean up all if related stuff. + * Notify all extcaps that capture session should be stopped. + * Forcefully stop session if extcaps do not finish before timeout. * @param cap_session Capture session. */ void -extcap_if_cleanup(capture_session *cap_session); +extcap_request_stop(capture_session *cap_session); /** * Fetch an extcap preference for a given argument. diff --git a/ui/capture.c b/ui/capture.c index 7baaf16e72..c0a14876fc 100644 --- a/ui/capture.c +++ b/ui/capture.c @@ -21,6 +21,7 @@ #include #include +#include "extcap.h" #include "file.h" #include "ui/capture.h" #include "capture/capture_ifinfo.h" @@ -182,8 +183,13 @@ capture_stop(capture_session *cap_session) capture_callback_invoke(capture_cb_capture_stopping, cap_session); - /* stop the capture child gracefully */ - sync_pipe_stop(cap_session); + if (!extcap_session_stop(cap_session)) { + extcap_request_stop(cap_session); + cap_session->capture_opts->stop_after_extcaps = TRUE; + } else { + /* stop the capture child gracefully */ + sync_pipe_stop(cap_session); + } }