GeoIP: avoid closing random file descriptors

Previously there were three different pipe validity checks:
PID != WS_INVALID_PID, PID != 0 and stdin != 0. This resulted in
using/closing file descriptors which might be owned by something else.

When no GeoIP databases are defined, mmdb_resolve_stop would be called
to close the pipe and set PID to WS_INVALID_PID. stdin is however not
cleared and future invocations would try to close the previous fd.

Change-Id: I1d15da29208efb41098ee6a4edeeabf61f84c2b3
Fixes: v2.5.1rc0-466-ga1da75c554 ("Transition from GeoIP Legacy to MaxMindDB.")
Reviewed-on: https://code.wireshark.org/review/26391
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Gerald Combs <gerald@wireshark.org>

2 files changed, 27 insertions, 6 deletions

diff --git a/epan/maxmind_db.c b/epan/maxmind_db.c
index ece13f819d..84fbb3a420 100644
--- a/epan/maxmind_db.c
+++ b/epan/maxmind_db.c
@@ -221,7 +221,10 @@ process_mmdbr_stdout(int fd) {
  * Stop our mmdbresolve process.
  */
 static void mmdb_resolve_stop(void) {
-    if (!mmdbr_pipe.pid) return;
+    if (!ws_pipe_valid(&mmdbr_pipe)) {
+        MMDB_DEBUG("not cleaning up, invalid PID %d", mmdbr_pipe.pid);
+        return;
+    }
 
     ws_close(mmdbr_pipe.stdin_fd);
     MMDB_DEBUG("closing pid %d", mmdbr_pipe.pid);
@@ -244,10 +247,18 @@ static void mmdb_resolve_start(void) {
         mmdb_str_chunk = wmem_map_new(wmem_epan_scope(), wmem_str_hash, g_str_equal);
     }
 
-    if (!mmdb_file_arr) return;
+    if (!mmdb_file_arr) {
+        MMDB_DEBUG("unexpected mmdb_file_arr == NULL");
+        return;
+    }
 
     mmdb_resolve_stop();
 
+    if (mmdb_file_arr->len == 0) {
+        MMDB_DEBUG("no GeoIP databases found");
+        return;
+    }
+
     GPtrArray *args = g_ptr_array_new();
     char *mmdbresolve = g_strdup_printf("%s%c%s", get_progfile_dir(), G_DIR_SEPARATOR, "mmdbresolve");
     g_ptr_array_add(args, mmdbresolve);
@@ -262,7 +273,9 @@ static void mmdb_resolve_start(void) {
     MMDB_DEBUG("spawned %s pid %d", mmdbresolve, pipe_pid);
 
     for (guint i = 0; i < args->len; i++) {
-        g_free(g_ptr_array_index(args, i));
+        char *arg = g_ptr_array_index(args, i);
+        MMDB_DEBUG("args: %s", arg);
+        g_free(arg);
     }
     g_ptr_array_free(args, TRUE);
 
@@ -405,7 +418,7 @@ void maxmind_db_pref_cleanup(void)
 
 gboolean maxmind_db_lookup_process(void)
 {
-    if (mmdbr_pipe.pid == WS_INVALID_PID) return FALSE;
+    if (!ws_pipe_valid(&mmdbr_pipe)) return FALSE;
 
     return process_mmdbr_stdout(mmdbr_pipe.stdout_fd);
 }
@@ -415,7 +428,7 @@ maxmind_db_lookup_ipv4(guint32 addr) {
     mmdb_lookup_t *result = (mmdb_lookup_t *) wmem_map_lookup(mmdb_ipv4_map, GUINT_TO_POINTER(addr));
 
     if (!result) {
-        if (mmdbr_pipe.stdin_fd) {
+        if (ws_pipe_valid(&mmdbr_pipe)) {
             char addr_str[WS_INET_ADDRSTRLEN + 1];
             ws_inet_ntop4(&addr, addr_str, WS_INET_ADDRSTRLEN);
             MMDB_DEBUG("looking up %s", addr_str);
@@ -439,7 +452,7 @@ maxmind_db_lookup_ipv6(const ws_in6_addr *addr) {
     mmdb_lookup_t * result = (mmdb_lookup_t *) wmem_map_lookup(mmdb_ipv6_map, addr->bytes);
 
     if (!result) {
-        if (mmdbr_pipe.stdin_fd) {
+        if (ws_pipe_valid(&mmdbr_pipe)) {
             char addr_str[WS_INET6_ADDRSTRLEN + 1];
             ws_inet_ntop6(addr, addr_str, WS_INET6_ADDRSTRLEN);
             MMDB_DEBUG("looking up %s", addr_str);
diff --git a/wsutil/ws_pipe.h b/wsutil/ws_pipe.h
index f0429d330d..a5f37a1040 100644
--- a/wsutil/ws_pipe.h
+++ b/wsutil/ws_pipe.h
@@ -58,6 +58,14 @@ WS_DLL_PUBLIC gboolean ws_pipe_spawn_sync ( gchar * dirname, gchar * command, gi
 WS_DLL_PUBLIC void ws_pipe_init(ws_pipe_t *ws_pipe);
 
 /**
+ * @brief Checks whether a pipe is valid (for reading or writing).
+ */
+static inline gboolean ws_pipe_valid(ws_pipe_t *ws_pipe)
+{
+    return ws_pipe && ws_pipe->pid && ws_pipe->pid != WS_INVALID_PID;
+}
+
+/**
  * @brief Start a process using g_spawn_sync on UNIX and Linux, and CreateProcess on Windows.
  * @param ws_pipe The process PID, stdio descriptors, etc.
  * @param args The command to run along with its arguments.