wsutil: Remove flawed ws_pipe_close() function

The semantics behind ws_pipe_close() were broken since its introduction.
Forcing process termination on Windows, while simply setting variable on
other systems results in more OS specific code sprinkled all over the
place instead of less. Moreover ws_pipe_close() never handled standard
file handles. It is really hard to come up with sensible ws_pipe_close()
replacement, as process exit is actually asynchronous action. It is
recommended to register child watch using g_child_watch_add() instead.

Do not call ws_pipe_close() when deleting capture interface. Things will
break if extcap is still running when interface opts are being freed and
terminating process won't help.

Rework maxmind shutdown to rely on GIOChannel state. For unknown reason
TerminateProcess() is still needed on Windows. The actual root cause
should be identified and fixed instead of giving up hope that it will
ever work correctly on Windows. In other words, TerminateProcess()
should not be used as a pattern, but rather as a last resort.

5 files changed, 17 insertions, 36 deletions

diff --git a/capture_opts.c b/capture_opts.c
index 53c1800bb7..498aaacfc0 100644
--- a/capture_opts.c
+++ b/capture_opts.c
@@ -1291,7 +1291,7 @@ capture_opts_del_iface(capture_options *capture_opts, guint if_index)
     if (interface_opts->extcap_args)
         g_hash_table_unref(interface_opts->extcap_args);
     if (interface_opts->extcap_pid != WS_INVALID_PID)
-        ws_pipe_close((ws_pipe_t *) interface_opts->extcap_pipedata);
+        ws_warning("Extcap still running during interface delete");
     g_free(interface_opts->extcap_pipedata);
     if (interface_opts->extcap_stderr)
         g_string_free(interface_opts->extcap_stderr, TRUE);
diff --git a/epan/maxmind_db.c b/epan/maxmind_db.c
index b128ae370e..b9a662e1b0 100644
--- a/epan/maxmind_db.c
+++ b/epan/maxmind_db.c
@@ -171,12 +171,6 @@ write_mmdbr_stdin_worker(gpointer data _U_) {
     MMDB_DEBUG("starting write worker");
 
     while (1) {
-        if (!mmdbr_pipe_valid()) {
-            // Should be due to mmdb_resolve_stop.
-            MMDB_DEBUG("invalid mmdbr stdin pipe. exiting thread.");
-            return NULL;
-        }
-
         // On some operating systems (most notably macOS), g_async_queue_timeout_pop
         // will return immediately if we've been built with an older version of GLib:
         //   https://bugzilla.gnome.org/show_bug.cgi?id=673607
@@ -184,10 +178,13 @@ write_mmdbr_stdin_worker(gpointer data _U_) {
         // mmdb_resolve_stop will close our pipe and then push an invalid address
         // (mmdbr_stop_sentinel) onto the queue.
         char *request = (char *) g_async_queue_pop(mmdbr_request_q);
-        if (!request || strcmp(request, mmdbr_stop_sentinel) == 0) {
-            g_free(request);
+        if (!request) {
             continue;
         }
+        if (strcmp(request, mmdbr_stop_sentinel) == 0) {
+            g_free(request);
+            return NULL;
+        }
 
         MMDB_DEBUG("write %s ql %d", request, g_async_queue_length(mmdbr_request_q));
         status = g_io_channel_write_chars(mmdbr_pipe.stdin_io, request, strlen(request), &bytes_written, &err);
@@ -248,12 +245,8 @@ read_mmdbr_stdout_worker(gpointer data _U_) {
                 if (bytes_read > 0) {
                     bytes_in_buffer += bytes_read;
                 } else {
-                    if (!mmdbr_pipe_valid()) {
-                        // Should be due to mmdb_resolve_stop.
-                        MMDB_DEBUG("invalid mmdbr stdout pipe. exiting thread.");
-                        break;
-                    }
-                    MMDB_DEBUG("no pipe data");
+                    MMDB_DEBUG("no pipe data. exiting thread.");
+                    break;
                 }
             } else {
                 MMDB_DEBUG("long line");
@@ -387,9 +380,6 @@ static void mmdb_resolve_stop(void) {
 
     g_rw_lock_writer_lock(&mmdbr_pipe_mtx);
 
-    MMDB_DEBUG("closing pid %d", mmdbr_pipe.pid);
-    ws_pipe_close(&mmdbr_pipe);
-
     g_async_queue_push(mmdbr_request_q, g_strdup(mmdbr_stop_sentinel));
 
     g_rw_lock_writer_unlock(&mmdbr_pipe_mtx);
@@ -401,6 +391,15 @@ static void mmdb_resolve_stop(void) {
     MMDB_DEBUG("closing stdin IO");
     g_io_channel_unref(mmdbr_pipe.stdin_io);
 
+#ifdef _WIN32
+    /* TODO: Actually solve the issue instead of just terminating process */
+    MMDB_DEBUG("terminating pid %d", mmdbr_pipe.pid);
+    TerminateProcess(mmdbr_pipe.pid, 0);
+#endif
+    MMDB_DEBUG("closing pid %d", mmdbr_pipe.pid);
+    g_spawn_close_pid(mmdbr_pipe.pid);
+    mmdbr_pipe.pid = WS_INVALID_PID;
+
     // child process notices broken stdin pipe and exits (breaks stdout pipe)
     // read_mmdbr_stdout_worker should exit
 
diff --git a/packaging/debian/libwsutil0.symbols b/packaging/debian/libwsutil0.symbols
index 97b885c00c..b033e3be4a 100644
--- a/packaging/debian/libwsutil0.symbols
+++ b/packaging/debian/libwsutil0.symbols
@@ -427,7 +427,6 @@ libwsutil.so.0 libwsutil0 #MINVER#
  ws_optopt@Base 3.5.1
  ws_optpos@Base 3.5.1
  ws_optreset@Base 3.5.1
- ws_pipe_close@Base 2.6.5
  ws_pipe_data_available@Base 2.5.0
  ws_pipe_init@Base 2.5.1
  ws_pipe_spawn_async@Base 2.5.1
diff --git a/wsutil/ws_pipe.c b/wsutil/ws_pipe.c
index cc998ba983..74c39103d0 100644
--- a/wsutil/ws_pipe.c
+++ b/wsutil/ws_pipe.c
@@ -653,17 +653,6 @@ GPid ws_pipe_spawn_async(ws_pipe_t *ws_pipe, GPtrArray *args)
     return pid;
 }
 
-void ws_pipe_close(ws_pipe_t * ws_pipe)
-{
-    if (ws_pipe->pid != WS_INVALID_PID) {
-#ifdef _WIN32
-        TerminateProcess(ws_pipe->pid, 0);
-#endif
-        g_spawn_close_pid(ws_pipe->pid);
-        ws_pipe->pid = WS_INVALID_PID;
-    }
-}
-
 #ifdef _WIN32
 
 typedef struct
diff --git a/wsutil/ws_pipe.h b/wsutil/ws_pipe.h
index e805e4317b..087e2bf013 100644
--- a/wsutil/ws_pipe.h
+++ b/wsutil/ws_pipe.h
@@ -69,12 +69,6 @@ static inline gboolean ws_pipe_valid(ws_pipe_t *ws_pipe)
  */
 WS_DLL_PUBLIC GPid ws_pipe_spawn_async (ws_pipe_t * ws_pipe, GPtrArray * args );
 
-/**
- * @brief Stop a process started with ws_pipe_spawn_async
- * @param ws_pipe The process PID, stdio descriptors, etc.
- */
-WS_DLL_PUBLIC void ws_pipe_close(ws_pipe_t * ws_pipe);
-
 #ifdef _WIN32
 /**
  * @brief Wait for a set of handles using WaitForMultipleObjects. Windows only.