View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0001602GNUnetobsoletepublic2010-09-09 21:492024-05-03 13:51
ReporterLRN Assigned ToNDurner  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Summary0001602: A patch to fix process spawning with redirected std streams
DescriptionModifies pipe creation function to make pipes inheritable or not inheritable depending on their further usage.
Adds a pseudo-cat program that copies stdin to stdout (no need to use MSys cat which is located is some unknown location).
TagsNo tags attached.
Attached Files
startprocess.diff (11,604 bytes)    
Index: src/arm/gnunet-service-arm.c
===================================================================
--- src/arm/gnunet-service-arm.c	(revision 12953)
+++ src/arm/gnunet-service-arm.c	(working copy)
@@ -1041,7 +1041,7 @@
   GNUNET_assert (serv != NULL);
   shc_chld = GNUNET_SIGNAL_handler_install (GNUNET_SIGCHLD, &sighandler_child_death);
   GNUNET_assert (sigpipe == NULL);
-  sigpipe = GNUNET_DISK_pipe (GNUNET_NO);
+  sigpipe = GNUNET_DISK_pipe (GNUNET_NO, GNUNET_NO, GNUNET_NO);
   GNUNET_assert (sigpipe != NULL);
   pr = GNUNET_DISK_pipe_handle (sigpipe, GNUNET_DISK_PIPE_END_READ);
   GNUNET_assert (pr != NULL);
Index: src/include/gnunet_disk_lib.h
===================================================================
--- src/include/gnunet_disk_lib.h	(revision 12953)
+++ src/include/gnunet_disk_lib.h	(working copy)
@@ -325,12 +325,18 @@
 
 /**
  * Creates an interprocess channel
+ *
  * @param blocking creates an asynchronous pipe if set to GNUNET_NO
+ * @param inherit_read 1 to make read handle inheritable, 0 otherwise (NT only)
+ * @param inherit_write 1 to make write handle inheritable, 0 otherwise (NT only)
  * @return handle to the new pipe, NULL on error
  */
-struct GNUNET_DISK_PipeHandle *GNUNET_DISK_pipe (int blocking);
+struct GNUNET_DISK_PipeHandle *GNUNET_DISK_pipe (int blocking,
+						 int inherit_read,
+						 int inherit_write);
 
 
+
 /**
  * Closes an interprocess channel
  * @param p pipe
Index: src/transport/plugin_transport_udp.c
===================================================================
--- src/transport/plugin_transport_udp.c	(revision 12953)
+++ src/transport/plugin_transport_udp.c	(working copy)
@@ -1627,7 +1627,7 @@
   if (plugin->behind_nat == GNUNET_YES)
     {
       /* Pipe to read from started processes stdout (on read end) */
-      plugin->server_stdout = GNUNET_DISK_pipe(GNUNET_YES);
+      plugin->server_stdout = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_NO, GNUNET_YES);
       if (plugin->server_stdout == NULL)
         return sockets_created;
 #if DEBUG_UDP
Index: src/transport/plugin_transport_tcp.c
===================================================================
--- src/transport/plugin_transport_tcp.c	(revision 12953)
+++ src/transport/plugin_transport_tcp.c	(working copy)
@@ -2140,7 +2140,7 @@
 tcp_transport_start_nat_server(struct Plugin *plugin)
 {
 
-  plugin->server_stdout = GNUNET_DISK_pipe(GNUNET_YES);
+  plugin->server_stdout = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_NO, GNUNET_YES);
   if (plugin->server_stdout == NULL)
     return GNUNET_SYSERR;
 
Index: src/transport/plugin_transport_wlan.c
===================================================================
--- src/transport/plugin_transport_wlan.c	(revision 12953)
+++ src/transport/plugin_transport_wlan.c	(working copy)
@@ -433,11 +433,11 @@
 wlan_transport_start_wlan_helper(struct Plugin *plugin)
 {
 
-  plugin->server_stdout = GNUNET_DISK_pipe(GNUNET_YES);
+  plugin->server_stdout = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_NO, GNUNET_YES);
   if (plugin->server_stdout == NULL)
     return GNUNET_SYSERR;
 
-  plugin->server_stdin = GNUNET_DISK_pipe(GNUNET_YES);
+  plugin->server_stdin = GNUNET_DISK_pipe(GNUNET_YES, GNUNET_YES, GNUNET_NO);
     if (plugin->server_stdin == NULL)
       return GNUNET_SYSERR;
 
Index: src/vpn/gnunet-vpn-pretty-print.c
===================================================================
--- src/vpn/gnunet-vpn-pretty-print.c	(revision 12953)
+++ src/vpn/gnunet-vpn-pretty-print.c	(working copy)
@@ -3,13 +3,11 @@
 #include <string.h>
 #include <ctype.h>
 #ifndef _WIN32
- #include <arpa/inet.h>
+#include <arpa/inet.h>
 #else
 #include <ws2tcpip.h>
 #endif
  
-#include <arpa/inet.h>
-
 #include "gnunet-vpn-packet.h"
 
 static char* pretty = /*{{{*/
Index: src/vpn/gnunet-daemon-vpn.c
===================================================================
--- src/vpn/gnunet-daemon-vpn.c	(revision 12953)
+++ src/vpn/gnunet-daemon-vpn.c	(working copy)
@@ -71,8 +71,8 @@
 static void helper_read(void* cls, const struct GNUNET_SCHEDULER_TaskContext* tsdkctx);
 
 static void start_helper_and_schedule() {
-	mycls.helper_in = GNUNET_DISK_pipe(GNUNET_YES);
-	mycls.helper_out = GNUNET_DISK_pipe(GNUNET_YES);
+	mycls.helper_in = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_YES, GNUNET_NO);
+	mycls.helper_out = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_NO, GNUNET_YES);
 
 	if (mycls.helper_in == NULL || mycls.helper_out == NULL) return;
 
Index: src/testing/testing.c
===================================================================
--- src/testing/testing.c	(revision 12953)
+++ src/testing/testing.c	(working copy)
@@ -241,7 +241,7 @@
       /* fall-through */
     case SP_COPIED:
       /* Start create hostkey process */
-      d->pipe_stdout = GNUNET_DISK_pipe(GNUNET_NO);
+      d->pipe_stdout = GNUNET_DISK_pipe (GNUNET_NO, GNUNET_NO, GNUNET_YES);
       if (d->pipe_stdout == NULL)
         {
           cb = d->cb;
Index: src/util/scheduler.c
===================================================================
--- src/util/scheduler.c	(revision 12953)
+++ src/util/scheduler.c	(working copy)
@@ -749,7 +749,7 @@
   rs = GNUNET_NETWORK_fdset_create ();
   ws = GNUNET_NETWORK_fdset_create ();
   GNUNET_assert (shutdown_pipe_handle == NULL);
-  shutdown_pipe_handle = GNUNET_DISK_pipe (GNUNET_NO);
+  shutdown_pipe_handle = GNUNET_DISK_pipe (GNUNET_NO, GNUNET_NO, GNUNET_NO);
   GNUNET_assert (shutdown_pipe_handle != NULL);
   pr = GNUNET_DISK_pipe_handle (shutdown_pipe_handle, GNUNET_DISK_PIPE_END_READ);
   GNUNET_assert (pr != NULL);
Index: src/util/os_priority.c
===================================================================
--- src/util/os_priority.c	(revision 12953)
+++ src/util/os_priority.c	(working copy)
@@ -255,13 +255,13 @@
   char *arg;
   unsigned int cmdlen;
   char *cmd, *idx;
+  int findresult;
   STARTUPINFO start;
   PROCESS_INFORMATION proc;
-#if NILS
+
   HANDLE stdin_handle;
   HANDLE stdout_handle;
-#endif
-  char *fn = NULL;
+
   char path[MAX_PATH + 1];
 
   cmdlen = 0;
@@ -270,7 +270,7 @@
       cmdlen = cmdlen + strlen (arg) + 3;
   va_end (ap);
 
-  cmd = idx = GNUNET_malloc (sizeof (char) * cmdlen);
+  cmd = idx = GNUNET_malloc (sizeof (char) * (cmdlen + 1));
   va_start (ap, filename);
   while (NULL != (arg = va_arg (ap, char *)))
       idx += sprintf (idx, "\"%s\" ", arg);
@@ -279,7 +279,6 @@
   memset (&start, 0, sizeof (start));
   start.cb = sizeof (start);
 
-#if NILS
   if ((pipe_stdin != NULL) || (pipe_stdout != NULL))
     start.dwFlags |= STARTF_USESTDHANDLES;
 
@@ -294,27 +293,26 @@
       GNUNET_DISK_internal_file_handle_ (GNUNET_DISK_pipe_handle(pipe_stdout, GNUNET_DISK_PIPE_END_WRITE), &stdout_handle, sizeof (HANDLE));
       start.hStdOutput = stdout_handle;
     }
-#endif
-  if ((int) FindExecutable(filename, NULL, path) <= 32) 
+
+  findresult = (int) FindExecutableA (filename, NULL, path);
+  if (findresult <= 32) 
     {
       SetErrnoFromWinError (GetLastError ());
-      GNUNET_log_strerror_file (GNUNET_ERROR_TYPE_ERROR, "FindExecutable", fn);
+      GNUNET_log_strerror_file (GNUNET_ERROR_TYPE_ERROR, "FindExecutable", filename);
       return -1;
     }
 
-  if (!CreateProcess
-      (path, cmd, NULL, NULL, FALSE, DETACHED_PROCESS, NULL, NULL, &start,
+  if (!CreateProcessA
+      (path, cmd, NULL, NULL, TRUE, DETACHED_PROCESS, NULL, NULL, &start,
        &proc))
     {
       SetErrnoFromWinError (GetLastError ());
-      GNUNET_log_strerror_file (GNUNET_ERROR_TYPE_ERROR, "CreateProcess", fn);
+      GNUNET_log_strerror_file (GNUNET_ERROR_TYPE_ERROR, "CreateProcess", path);
       return -1;
     }
 
   CreateThread (NULL, 64000, ChildWaitThread, proc.hProcess, 0, NULL);
 
-  if (fn != filename)
-    GNUNET_free (fn);
   CloseHandle (proc.hThread);
 
   GNUNET_free (cmd);
Index: src/util/test_os_start_process.c
===================================================================
--- src/util/test_os_start_process.c	(revision 12953)
+++ src/util/test_os_start_process.c	(working copy)
@@ -98,7 +98,6 @@
 
 }
 
-
 static void
 task (void *cls, const struct GNUNET_SCHEDULER_TaskContext *tc)
 {
@@ -106,10 +105,14 @@
   const struct GNUNET_DISK_FileHandle *stdout_read_handle;
   const struct GNUNET_DISK_FileHandle *wh;
 
+#ifndef WINDOWS
   GNUNET_asprintf(&fn, "cat");
+#else
+  GNUNET_asprintf(&fn, "./.libs/test_os_start_process_cat.exe");
+#endif
 
-  hello_pipe_stdin = GNUNET_DISK_pipe(GNUNET_YES);
-  hello_pipe_stdout = GNUNET_DISK_pipe(GNUNET_YES);
+  hello_pipe_stdin = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_YES, GNUNET_NO);
+  hello_pipe_stdout = GNUNET_DISK_pipe (GNUNET_YES, GNUNET_NO, GNUNET_YES);
 
   if ((hello_pipe_stdout == NULL) || (hello_pipe_stdin == NULL))
     {
Index: src/util/disk.c
===================================================================
--- src/util/disk.c	(revision 12953)
+++ src/util/disk.c	(working copy)
@@ -1646,10 +1646,12 @@
  * Creates an interprocess channel
  *
  * @param blocking creates an asynchronous pipe if set to GNUNET_NO
+ * @param inherit_read 1 to make read handle inheritable, 0 otherwise (NT only)
+ * @param inherit_write 1 to make write handle inheritable, 0 otherwise (NT only)
  * @return handle to the new pipe, NULL on error
  */
 struct GNUNET_DISK_PipeHandle *
-GNUNET_DISK_pipe (int blocking)
+GNUNET_DISK_pipe (int blocking, int inherit_read, int inherit_write)
 {
   struct GNUNET_DISK_PipeHandle *p;
   struct GNUNET_DISK_FileHandle *fds;
@@ -1699,6 +1701,7 @@
     }
 #else
   BOOL ret;
+  HANDLE tmp_handle;
 
   ret = CreatePipe (&p->fd[0]->h, &p->fd[1]->h, NULL, 0);
   if (!ret)
@@ -1707,6 +1710,33 @@
       SetErrnoFromWinError (GetLastError ());
       return NULL;
     }
+
+  if (!DuplicateHandle (GetCurrentProcess (), p->fd[0]->h,
+			GetCurrentProcess (), &tmp_handle, 0, inherit_read == GNUNET_YES ? TRUE : FALSE,
+			DUPLICATE_SAME_ACCESS))
+  {
+    SetErrnoFromWinError (GetLastError ());
+    CloseHandle (p->fd[0]->h);
+    CloseHandle (p->fd[1]->h);
+    GNUNET_free (p);
+    return NULL;
+  }
+  CloseHandle (p->fd[0]->h);
+  p->fd[0]->h = tmp_handle;
+  
+  if (!DuplicateHandle (GetCurrentProcess (), p->fd[1]->h,
+			GetCurrentProcess (), &tmp_handle, 0, inherit_write == GNUNET_YES ? TRUE : FALSE,
+			DUPLICATE_SAME_ACCESS))
+  {
+    SetErrnoFromWinError (GetLastError ());
+    CloseHandle (p->fd[0]->h);
+    CloseHandle (p->fd[1]->h);
+    GNUNET_free (p);
+    return NULL;
+  }
+  CloseHandle (p->fd[1]->h);
+  p->fd[1]->h = tmp_handle;
+  
   if (!blocking)
     {
       DWORD mode;
@@ -1852,7 +1882,6 @@
     }
 }
 
-
 /**
  * Retrieve OS file handle
  * @internal
Index: src/util/test_scheduler.c
===================================================================
--- src/util/test_scheduler.c	(revision 12953)
+++ src/util/test_scheduler.c	(working copy)
@@ -114,7 +114,7 @@
   int *ok = cls;
   GNUNET_assert (5 == *ok);
   (*ok) = 6;
-  p = GNUNET_DISK_pipe (GNUNET_NO);
+  p = GNUNET_DISK_pipe (GNUNET_NO, GNUNET_NO, GNUNET_NO);
   GNUNET_assert (NULL != p);
   fds[0] = GNUNET_DISK_pipe_handle (p, GNUNET_DISK_PIPE_END_READ);
   fds[1] = GNUNET_DISK_pipe_handle (p, GNUNET_DISK_PIPE_END_WRITE);
Index: src/util/Makefile.am
===================================================================
--- src/util/Makefile.am	(revision 12953)
+++ src/util/Makefile.am	(working copy)
@@ -14,6 +14,9 @@
   -lshell32 -liconv -lstdc++ \
   -lcomdlg32 -lgdi32
 WINLIB = libgnunetutilwin.la
+
+noinst_PROGRAMS = test_os_start_process_cat
+test_os_start_process_cat_SOURCES = test_os_start_process_cat.c
 endif
 
 if USE_COVERAGE
startprocess.diff (11,604 bytes)   
test_os_start_process_cat.c (209 bytes)    
#include <stdio.h>
#include <windows.h>

int
main (int argc, char *argv[])
{
  char c;
  int ret = 0;
  while (fread (&c, 1, 1, stdin) == 1)
    ret = ret & (!(fwrite (&c, 1, 1, stdout) == 1));
  return ret;
}
test_os_start_process_cat.c (209 bytes)   
pipetests.tar.xz (3,484 bytes)

Activities

NDurner

2010-09-12 12:19

reporter   ~0004117

Thanks for the patch.

> +struct GNUNET_DISK_PipeHandle *GNUNET_DISK_pipe (int blocking,
> + int inherit_read,
> + int inherit_write);

Can't we just inherit all handles?


> +test_os_start_process_cat_SOURCES = test_os_start_process_cat.c

This source file is missing.
Anyway, why are gcc & co in your path and cat isn't?

LRN

2010-09-12 13:23

reporter   ~0004118

Uploaded test_os_start_process_cat.c, though it is not necessary anymore. I needed a custom pseudo-cat to check the state of the child process after it is spawned (debugging real cat is awkward). It should work with simple "cat" now.
At first i thought that it fails to find "cat.exe" and changed to pseudo-cat, but later i discovered that you've used Unicode variant of FindExecutable, which is why it failed to find anything at all.

As for handle inheritance:
1) It is insecure to give child process handles it shouldn't use, therefore handles should be created inheritable only when they are needed to be inheritable.
2) I didn't like the idea of writing a separate function that would make handles inheritable/uninheritable at random point of time. Because such operation involves duplicating a handle and closing the original, it might break things when original handle is already in use somewhere. It's easier to decide whether a handle is inheritable or not right at creation time.
It *might* be a good idea to make GNUNET_DISK_pipe() a vararg function to make inherit_read and inherit_write optional.

NDurner

2010-09-12 23:36

reporter   ~0004119

(13:50:11) LRN: I think i remember reading somewhere that giving a handle to a child process prevents its closure and might break things.
(13:51:23) LRN: http://support.microsoft.com/kb/190351 also refers to that, but in a very vague way
(13:56:38) LRN: Also, a story of non-success - http://aautar.digital-radiation.com/blog/?p=407

LRN

2010-09-15 12:42

reporter   ~0004120

I've uploaded pipetests.tar.xz
How to use:
Build pipemaster.exe and pipeslave.exe (run build.sh from MSys if you're lazy)
Run a command interpreter (cmd.exe)
CD into directory with pipemaster.exe and pipeslave.exe
Run:
pipemaster.exe <notinherit|inherit>

pipemaster inherits cmd.exe's console, pipeslave gets a new console from CreateProcess (to avoid race conditions in using console's stderr for debug output).

pipemaster reads string from the console input, then writes it into pipeslave's stdin pipe. Pipeslave reads string from its stdin pipe and writes "echo %s" into its stdout pipe. Pipemaster reads echoed string from pipeslave's stdout pipe. And so on. The process ends when the input string starts with "Exit" or "Quit". "Exit" makes pipeslave exit, while "Quit" makes pipemaster exit.

The difference between inherit and noninherit modes is that "Quit" will hang pipeslave in inherit mode, because its stding pipe write end is duplicated into pipeslave itself (the handle is inheritable). In noninherit mode when pipemaster closes that handle, pipeslave's fgets() returns NULL and it is able to exit too. In inherit mode a duplicated handle to stdin pipe write end still exists in pipeslave and fgets() keeps waiting forever.

Matthias Wachs

2010-09-15 13:13

reporter   ~0004121

Added patch to svn

Issue History

Date Modified Username Field Change
2010-09-09 21:49 LRN New Issue
2010-09-09 21:49 LRN Status new => assigned
2010-09-09 21:49 LRN Assigned To => NDurner
2010-09-09 21:49 LRN File Added: startprocess.diff
2010-09-12 12:19 NDurner Note Added: 0004117
2010-09-12 12:48 LRN File Added: test_os_start_process_cat.c
2010-09-12 13:23 LRN Note Added: 0004118
2010-09-12 23:36 NDurner Note Added: 0004119
2010-09-15 12:35 LRN File Added: pipetests.tar.xz
2010-09-15 12:42 LRN Note Added: 0004120
2010-09-15 13:13 Matthias Wachs Note Added: 0004121
2010-09-20 11:31 Matthias Wachs Resolution open => fixed
2010-10-21 15:40 Christian Grothoff Status assigned => resolved
2010-10-21 15:47 Christian Grothoff Status resolved => closed
2024-01-12 14:28 schanzen Category Win32 port => Win32 port (deprecated)
2024-05-03 13:51 Christian Grothoff Category Win32 port (deprecated) => obsolete