From a395a8b9939bd48858efbd3da5897a59d4697eba Mon Sep 17 00:00:00 2001 From: Peter Wu Date: Fri, 9 Mar 2018 15:17:02 +0100 Subject: 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 Tested-by: Petri Dish Buildbot Reviewed-by: Gerald Combs --- epan/maxmind_db.c | 25 +++++++++++++++++++------ wsutil/ws_pipe.h | 8 ++++++++ 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 @@ -57,6 +57,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. -- cgit v1.2.3