osmocom
/
wireshark
11
0
Fork 1
Code Issues Pull Requests Projects Releases Wiki Activity

win32-utils: Explicitly list inherited handles 

Windows processes inherit all inheritable handles when a new process is
created using CreateProcess() with bInheritHandles set to TRUE. This can
lead to undesired object lifetime extension. That is, the child process
will keep ineritable handles alive even if it does not use them. Up to
Windows Vista it was not possible explicitly list handles that should be
inherited. Wireshark no longer works on Windows releases earlier than
Vista, so use the new API without checking Windows version.

Require all callers to win32_create_process() to pass in the list of
handles to inherit. Set the listed handles as inheritable shortly before
calling CreateProcess() and set them as not inheritable shortly after
the process is created. This minimizes possibility for other callers
(especially in 3rd party libraries) to inherit handles by accident.

Do not terminate mmdbresolve process on exit. Instead rely on process
exit when EOF is received on standard input. Previously the EOF was
never received because mmdbresolve inherited both ends of standard input
pipe, i.e. the fact that Wireshark closed the write end was not observed
by mmdbresolve because mmdbresolve kept write handle the standard input
pipe open.
This commit is contained in:
Tomasz Moń 2022-08-15 10:51:20 +02:00
parent 3c3d715628
commit c6ef99f006
No known key found for this signature in database
GPG Key ID: 92BA8820D4D517C8
7 changed files with 111 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
capture/capture_sync.c
View File

@ -321,6 +321,8 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
void (*update_cb)(void))
{
#ifdef _WIN32
size_t i_handles = 0; /* Number of handles the child prcess will inherit */
HANDLE *handles; /* Handles the child process will inherit */
HANDLE sync_pipe_read; /* pipe used to send messages from child to parent */
HANDLE sync_pipe_write; /* pipe used to send messages from child to parent */
int signal_pipe_write_fd;
@ -477,6 +479,7 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
char *pipe = ws_strdup_printf("%s%" PRIuPTR, EXTCAP_PIPE_PREFIX, interface_opts->extcap_pipe_h);
argv = sync_pipe_add_arg(argv, &argc, pipe);
g_free(pipe);
i_handles++;
#else
argv = sync_pipe_add_arg(argv, &argc, interface_opts->extcap_fifo);
#endif
@ -595,7 +598,7 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
#ifdef _WIN32
/* init SECURITY_ATTRIBUTES */
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.bInheritHandle = TRUE;
sa.bInheritHandle = FALSE;
sa.lpSecurityDescriptor = NULL;
/* Create a pipe for the child process */
@ -685,8 +688,24 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
g_free(quoted_arg);
}
handles = g_new(HANDLE, 3 + i_handles);
i_handles = 0;
if (si.hStdInput) {
handles[i_handles++] = si.hStdInput;
}
if (si.hStdOutput && (si.hStdOutput != si.hStdInput)) {
handles[i_handles++] = si.hStdOutput;
}
handles[i_handles++] = si.hStdError;
for (j = 0; j < capture_opts->ifaces->len; j++) {
interface_opts = &g_array_index(capture_opts->ifaces, interface_options, j);
if (interface_opts->extcap_fifo != NULL) {
handles[i_handles++] = interface_opts->extcap_pipe_h;
}
}
/* call dumpcap */
if(!win32_create_process(argv[0], args->str, NULL, NULL, TRUE,
if(!win32_create_process(argv[0], args->str, NULL, NULL, i_handles, handles,
CREATE_NEW_CONSOLE, NULL, NULL, &si, &pi)) {
report_failure("Couldn't run %s in child process: %s",
args->str, win32strerror(GetLastError()));
@ -695,12 +714,14 @@ sync_pipe_start(capture_options *capture_opts, GPtrArray *capture_comments,
ws_close(signal_pipe_write_fd); /* Should close signal_pipe */
free_argv(argv, argc);
g_string_free(args, TRUE);
g_free(handles);
return FALSE;
}
cap_session->fork_child = pi.hProcess;
/* We may need to store this and close it later */
CloseHandle(pi.hThread);
g_string_free(args, TRUE);
g_free(handles);
cap_session->signal_pipe_write_fd = signal_pipe_write_fd;
@ -816,6 +837,7 @@ sync_pipe_open_command(char* const argv[], int *data_read_fd,
#ifdef _WIN32
HANDLE sync_pipe[2]; /* pipe used to send messages from child to parent */
HANDLE data_pipe[2]; /* pipe used to send data from child to parent */
HANDLE handles[2]; /* handles inherited by child process */
GString *args = g_string_sized_new(200);
gchar *quoted_arg;
SECURITY_ATTRIBUTES sa;
@ -843,7 +865,7 @@ sync_pipe_open_command(char* const argv[], int *data_read_fd,
#ifdef _WIN32
/* init SECURITY_ATTRIBUTES */
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.bInheritHandle = TRUE;
sa.bInheritHandle = FALSE;
sa.lpSecurityDescriptor = NULL;
/* Create a pipe for the child process to send us messages */
@ -914,6 +936,9 @@ sync_pipe_open_command(char* const argv[], int *data_read_fd,
si.hStdError = sync_pipe[PIPE_WRITE];
#endif
handles[0] = si.hStdOutput;
handles[1] = si.hStdError;
/* convert args array into a single string */
/* XXX - could change sync_pipe_add_arg() instead */
/* there is a drawback here: the length is internally limited to 1024 bytes */
@ -925,8 +950,8 @@ sync_pipe_open_command(char* const argv[], int *data_read_fd,
}
/* call dumpcap */
if(!win32_create_process(argv[0], args->str, NULL, NULL, TRUE,
CREATE_NEW_CONSOLE, NULL, NULL, &si, &pi)) {
if(!win32_create_process(argv[0], args->str, NULL, NULL, G_N_ELEMENTS(handles), handles,
CREATE_NEW_CONSOLE, NULL, NULL, &si, &pi)) {
*msg = ws_strdup_printf("Couldn't run %s in child process: %s",
args->str, win32strerror(GetLastError()));
ws_close(*data_read_fd); /* Should close data_pipe[PIPE_READ] */

5
epan/maxmind_db.c
View File

@ -391,11 +391,6 @@ 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;

2
extcap.c
View File

@ -1623,7 +1623,7 @@ static gboolean extcap_create_pipe(const gchar *ifname, gchar **fifo, HANDLE *ha
/* Security struct to enable Inheritable HANDLE */
memset(&security, 0, sizeof(SECURITY_ATTRIBUTES));
security.nLength = sizeof(SECURITY_ATTRIBUTES);
security.bInheritHandle = TRUE;
security.bInheritHandle = FALSE;
security.lpSecurityDescriptor = NULL;
/* create a namedPipe */

10
sharkd_daemon.c
View File

@ -372,6 +372,8 @@ sharkd_loop(int argc _U_, char* argv[])
#ifndef _WIN32
pid_t pid;
#else
size_t i_handles;
HANDLE handles[2];
PROCESS_INFORMATION pi;
STARTUPINFO si;
char *exename;
@ -415,6 +417,12 @@ sharkd_loop(int argc _U_, char* argv[])
si.hStdOutput = (HANDLE) fd;
si.hStdError = GetStdHandle(STD_ERROR_HANDLE);
i_handles = 0;
handles[i_handles++] = (HANDLE)fd;
if (si.hStdError != NULL) {
handles[i_handles++] = si.hStdError;
}
exename = ws_strdup_printf("%s\\%s", get_progfile_dir(), "sharkd.exe");
// we need to pass in all of the command line parameters except the -a parameter
@ -448,7 +456,7 @@ sharkd_loop(int argc _U_, char* argv[])
}
}
if (!win32_create_process(exename, command_line, NULL, NULL, TRUE, 0, NULL, NULL, &si, &pi))
if (!win32_create_process(exename, command_line, NULL, NULL, i_handles, handles, 0, NULL, NULL, &si, &pi))
{
fprintf(stderr, "win32_create_process(%s) failed\n", exename);
}

48
wsutil/win32-utils.c
View File

@ -219,10 +219,13 @@ static void win32_kill_child_on_exit(HANDLE child_handle) {
}
}
BOOL win32_create_process(const char *application_name, const char *command_line, LPSECURITY_ATTRIBUTES process_attributes, LPSECURITY_ATTRIBUTES thread_attributes, BOOL inherit_handles, DWORD creation_flags, LPVOID environment, const char *current_directory, LPSTARTUPINFO startup_info, LPPROCESS_INFORMATION process_information)
BOOL win32_create_process(const char *application_name, const char *command_line, LPSECURITY_ATTRIBUTES process_attributes, LPSECURITY_ATTRIBUTES thread_attributes, size_t n_inherit_handles, HANDLE *inherit_handles, DWORD creation_flags, LPVOID environment, const char *current_directory, LPSTARTUPINFO startup_info, LPPROCESS_INFORMATION process_information)
{
gunichar2 *wappname = NULL, *wcurrentdirectory = NULL;
gunichar2 *wcommandline = g_utf8_to_utf16(command_line, -1, NULL, NULL, NULL);
LPPROC_THREAD_ATTRIBUTE_LIST attribute_list = NULL;
STARTUPINFOEX startup_infoex;
size_t i;
// CREATE_SUSPENDED: Suspend the child so that we can cleanly call
// AssignProcessToJobObject.
DWORD wcreationflags = creation_flags|CREATE_SUSPENDED;
@ -243,15 +246,54 @@ BOOL win32_create_process(const char *application_name, const char *command_line
if (current_directory) {
wcurrentdirectory = g_utf8_to_utf16(current_directory, -1, NULL, NULL, NULL);
}
if (n_inherit_handles > 0) {
size_t attr_size = 0;
BOOL success;
success = InitializeProcThreadAttributeList(NULL, 1, 0, &attr_size);
if (success || (GetLastError() == ERROR_INSUFFICIENT_BUFFER)) {
attribute_list = g_malloc(attr_size);
success = InitializeProcThreadAttributeList(attribute_list, 1, 0, &attr_size);
}
if (success && (attribute_list != NULL)) {
success = UpdateProcThreadAttribute(attribute_list, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
inherit_handles, n_inherit_handles * sizeof(HANDLE), NULL, NULL);
}
if (!success && (attribute_list != NULL)) {
DeleteProcThreadAttributeList(attribute_list);
g_free(attribute_list);
attribute_list = NULL;
}
}
memset(&startup_infoex, 0, sizeof(startup_infoex));
startup_infoex.StartupInfo = *startup_info;
startup_infoex.StartupInfo.cb = sizeof(startup_infoex);
startup_infoex.lpAttributeList = attribute_list;
wcreationflags |= EXTENDED_STARTUPINFO_PRESENT;
for (i = 0; i < n_inherit_handles; i++) {
SetHandleInformation(inherit_handles[i], HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT);
}
BOOL cp_res = CreateProcess(wappname, wcommandline, process_attributes, thread_attributes,
inherit_handles, wcreationflags, environment, wcurrentdirectory, startup_info,
process_information);
(n_inherit_handles > 0) ? TRUE : FALSE, wcreationflags, environment, wcurrentdirectory,
&startup_infoex.StartupInfo, process_information);
/* While this function makes the created process inherit only the explicitly
* listed handles, there can be other functions (in 3rd party libraries)
* that create processes inheriting all inheritable handles. To minimize
* number of unwanted handle duplicates (handle duplicate can extend object
* lifetime, e.g. pipe write end) created that way clear the inherit flag.
*/
for (i = 0; i < n_inherit_handles; i++) {
SetHandleInformation(inherit_handles[i], HANDLE_FLAG_INHERIT, 0);
}
if (cp_res) {
win32_kill_child_on_exit(process_information->hProcess);
ResumeThread(process_information->hThread);
}
// XXX Else try again if CREATE_BREAKAWAY_FROM_JOB and GetLastError() == ERROR_ACCESS_DENIED?
if (attribute_list) {
DeleteProcThreadAttributeList(attribute_list);
g_free(attribute_list);
}
g_free(wappname);
g_free(wcommandline);
g_free(wcurrentdirectory);

13
wsutil/win32-utils.h
View File

@ -60,11 +60,19 @@ const char * win32strexception(DWORD exception);
/**
* @brief ws_pipe_create_process Create a process and assign it to the main application
* job object so that it will be killed when the main application exits.
*
* In order to limit unwanted handle duplicates in subprocesses all handles should be
* created as not inheritable and passed in the inherit_handles array. This function
* marks the handles as inheritable for as short time as possible. Note that handles
* passed to this function will have the inheritable flag cleared on exit. Processes
* created with this function inherit only the provided handles.
*
* @param application_name Application name. Will be converted to its UTF-16 equivalent or NULL.
* @param command_line Command line. Will be converted to its UTF-16 equivalent.
* @param process_attributes Same as CreateProcess.
* @param thread_attributes Same as CreateProcess.
* @param inherit_handles Same as CreateProcess.
* @param n_inherit_handles Number of handles the child process will inherit.
* @param inherit_handles Handles the child process will inherit.
* @param creation_flags Will be ORed with CREATE_SUSPENDED|CREATE_BREAKAWAY_FROM_JOB.
* @param environment Same as CreateProcess.
* @param current_directory Current directory. Will be converted to its UTF-16 equivalent or NULL.
@ -75,7 +83,8 @@ const char * win32strexception(DWORD exception);
WS_DLL_PUBLIC
BOOL win32_create_process(const char *application_name, const char *command_line,
LPSECURITY_ATTRIBUTES process_attributes, LPSECURITY_ATTRIBUTES thread_attributes,
BOOL inherit_handles, DWORD creation_flags, LPVOID environment,
size_t n_inherit_handles, HANDLE *inherit_handles,
DWORD creation_flags, LPVOID environment,
const char *current_directory, LPSTARTUPINFO startup_info, LPPROCESS_INFORMATION process_information
);

19
wsutil/ws_pipe.c
View File

@ -243,6 +243,7 @@ gboolean ws_pipe_spawn_sync(const gchar *working_directory, const gchar *command
HANDLE child_stdout_wr = NULL;
HANDLE child_stderr_rd = NULL;
HANDLE child_stderr_wr = NULL;
HANDLE inherit_handles[2];
OVERLAPPED stdout_overlapped;
OVERLAPPED stderr_overlapped;
@ -281,7 +282,7 @@ gboolean ws_pipe_spawn_sync(const gchar *working_directory, const gchar *command
memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.bInheritHandle = TRUE;
sa.bInheritHandle = FALSE;
sa.lpSecurityDescriptor = NULL;
if (!ws_pipe_create_overlapped_read(&child_stdout_rd, &child_stdout_wr, &sa, 0))
@ -306,6 +307,9 @@ gboolean ws_pipe_spawn_sync(const gchar *working_directory, const gchar *command
return FALSE;
}
inherit_handles[0] = child_stderr_wr;
inherit_handles[1] = child_stdout_wr;
memset(&processInfo, 0, sizeof(PROCESS_INFORMATION));
memset(&info, 0, sizeof(STARTUPINFO));
@ -315,7 +319,8 @@ gboolean ws_pipe_spawn_sync(const gchar *working_directory, const gchar *command
info.dwFlags = STARTF_USESTDHANDLES | STARTF_USESHOWWINDOW;
info.wShowWindow = SW_HIDE;
if (win32_create_process(NULL, command_line, NULL, NULL, TRUE, CREATE_NEW_CONSOLE, NULL, working_directory, &info, &processInfo))
if (win32_create_process(NULL, command_line, NULL, NULL, G_N_ELEMENTS(inherit_handles), inherit_handles,
CREATE_NEW_CONSOLE, NULL, working_directory, &info, &processInfo))
{
gchar* stdout_buffer = (gchar*)g_malloc(BUFFER_SIZE);
gchar* stderr_buffer = (gchar*)g_malloc(BUFFER_SIZE);
@ -528,6 +533,7 @@ GPid ws_pipe_spawn_async(ws_pipe_t *ws_pipe, GPtrArray *args)
HANDLE child_stdout_wr = NULL;
HANDLE child_stderr_rd = NULL;
HANDLE child_stderr_wr = NULL;
HANDLE inherit_handles[3];
#endif
// XXX harmonize handling of command arguments for the sync/async functions
@ -540,7 +546,7 @@ GPid ws_pipe_spawn_async(ws_pipe_t *ws_pipe, GPtrArray *args)
#ifdef _WIN32
sa.nLength = sizeof(SECURITY_ATTRIBUTES);
sa.bInheritHandle = TRUE;
sa.bInheritHandle = FALSE;
sa.lpSecurityDescriptor = NULL;
if (!CreatePipe(&child_stdin_rd, &child_stdin_wr, &sa, 0))
@ -573,6 +579,10 @@ GPid ws_pipe_spawn_async(ws_pipe_t *ws_pipe, GPtrArray *args)
return WS_INVALID_PID;
}
inherit_handles[0] = child_stdin_rd;
inherit_handles[1] = child_stderr_wr;
inherit_handles[2] = child_stdout_wr;
memset(&processInfo, 0, sizeof(PROCESS_INFORMATION));
memset(&info, 0, sizeof(STARTUPINFO));
@ -583,7 +593,8 @@ GPid ws_pipe_spawn_async(ws_pipe_t *ws_pipe, GPtrArray *args)
info.dwFlags = STARTF_USESTDHANDLES | STARTF_USESHOWWINDOW;
info.wShowWindow = SW_HIDE;
if (win32_create_process(NULL, command_line, NULL, NULL, TRUE, CREATE_NEW_CONSOLE, NULL, NULL, &info, &processInfo))
if (win32_create_process(NULL, command_line, NULL, NULL, G_N_ELEMENTS(inherit_handles), inherit_handles,
CREATE_NEW_CONSOLE, NULL, NULL, &info, &processInfo))
{
stdin_fd = _open_osfhandle((intptr_t)(child_stdin_wr), _O_BINARY);
stdout_fd = _open_osfhandle((intptr_t)(child_stdout_rd), _O_BINARY);