This is the mail archive of the cygwin mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] patch for run.exe -- ATTN: ago



The attached patch seems to solve all the issues revealed so far. There is one open question, which may dictate a later change but the current patch should work as desired on all platforms, and is suitable for run-1.1.10 right now, IMO.


Here's the open question: I can find no reliable mechanism for determining that a given HANDLE returned by GetStdHandle *actually* refers to a true win32 cmd.exe console. Doing things like calling CreateFile() on "CONIN$" and comparing the values doesn't work, and we don't necessarily want to do that anyway: if CONIN$ is closed, we don't want to reopen it. Calling functions that *ought* only to work if the passed HANDLE is a console IO handle don't work either -- these functions succeed when (in my naivete) I believe they should fail! So, I punted. If we DO find a workable mechanism -- and for some reason we decide we care[*] -- there are two simple code changes needed to incorporate that future knowledge, and those are detailed in the embedded comments.

[*] Really, why should run.exe try to maintain a connection to a calling console? (a) it has no --help output (and if it did, it would probably use the message() function which formats text in a windows popup anyway) (b) it never tries to output or input anything from the console itself (c) the whole point is to hide whatever its client might try to input or output from a console. Actually, I think calling FreeConsole() right off the bat is the *right* thing for run.exe to do...unless it is shown to cause a problem in some corner case I haven't yet discovered.

So, the attached patch unconditionally and in all cases calls FreeConsole() to dissociate from any existing one. Then, it tries to create an invisible console. If that fails, then it falls back to using pipes as ago suggested, only I've pulled that code into a different function and modified it a bit. (You can force even NT/2K/XP to use the pipes by compiling with -DDEBUG_FORCE_PIPES).

In either mode -- invisible console or using pipes -- rxvt-unicde-X is happy, whether run.exe is invoked via bash-in-rxvt, via bash-in-cmd, or via a shortcut, and whether -wait is used or not.

(I also converted C++ comments to C).

--
Chuck

diff -urN -x .build -x .inst -x .sinst -x .buildlogs run-1.1.9-orig/src/run.c run-1.1.9/src/run.c
--- run-1.1.9-orig/src/run.c	2006-04-06 14:10:55.000000000 -0400
+++ run-1.1.9/src/run.c	2006-05-21 15:21:34.546875000 -0400
@@ -32,6 +32,7 @@
 #define WIN32
 
 #include <windows.h>
+#include <winuser.h>
 #include <string.h>
 #include <malloc.h>
 #include <stdlib.h>
@@ -161,7 +162,7 @@
    xemacs_special(exec);
    ret_code = start_child(cmdline2,wait_for_child);
    if (compact_invocation)
-      for (i = 1; i < argc; i++) // argv[0] was not malloc'ed
+      for (i = 1; i < argc; i++) /* argv[0] was not malloc'ed */
          free(argv[i]);
    else
       for (i = 0; i < argc; i++)
@@ -193,42 +194,240 @@
         free(var);
     }
 }
+BOOL setup_invisible_console()
+{
+   HWINSTA h, horig;
+   USEROBJECTFLAGS oi;
+   DWORD len;
+   BOOL b = FALSE; 
+   HMODULE lib = NULL;
+   HWINSTA WINAPI (*GetProcessWindowStationFP)(void) = NULL;
+   BOOL WINAPI (*GetUserObjectInformationFP)(HANDLE, int, PVOID, DWORD, PDWORD) = NULL;
+   HWINSTA WINAPI (*CreateWindowStationFP)(LPCWSTR, DWORD, DWORD, LPSECURITY_ATTRIBUTES) = NULL;
+   BOOL WINAPI (*SetProcessWindowStationFP)(HWINSTA) = NULL;
+   BOOL WINAPI (*CloseWindowStationFP)(HWINSTA) = NULL;
+
+   /* until we have a mechanism of determining whether a given HANDLE 
+    * returned by GetStdHandles actually derives from a console, 
+    * unconditionally call FreeConsole() on all OSes under all conditions.
+    * See comments in configure_startupinfo(). 
+    */
+   FreeConsole();
 
+   /* First, set up function pointers */
+   if (lib = LoadLibrary ("user32.dll"))
+   {
+       GetProcessWindowStationFP = (HWINSTA WINAPI (*)(void))
+           GetProcAddress (lib, "GetProcessWindowStation");
+       GetUserObjectInformationFP = (BOOL WINAPI (*)(HANDLE, int, PVOID, DWORD, PDWORD))
+           GetProcAddress (lib, "GetUserObjectInformationW"); /* ugly! */
+       CreateWindowStationFP = (HWINSTA WINAPI (*)(LPCWSTR, DWORD, DWORD, LPSECURITY_ATTRIBUTES))
+           GetProcAddress (lib, "CreateWindowStationW"); /* ugly */
+       SetProcessWindowStationFP = (BOOL WINAPI (*)(HWINSTA))
+           GetProcAddress (lib, "SetProcessWindowStation");
+       CloseWindowStationFP = (BOOL WINAPI (*)(HWINSTA))
+           GetProcAddress (lib, "CloseWindowStation");
+
+       if (GetProcessWindowStationFP && 
+            GetUserObjectInformationFP &&
+            CreateWindowStationFP &&
+            SetProcessWindowStationFP &&
+            CloseWindowStationFP)
+       {
+           /* Then, do the work */
+           FreeConsole();
+           h = horig = (*GetProcessWindowStationFP)();
+           if (!horig 
+               || !(*GetUserObjectInformationFP) (horig, UOI_FLAGS, &oi, sizeof (oi), &len )
+               || !(oi.dwFlags & WSF_VISIBLE))
+           {
+               b = AllocConsole();
+           }
+           else
+           {
+               h = (*CreateWindowStationFP) (NULL, 0, STANDARD_RIGHTS_READ, NULL);
+               if (h)
+               {
+                   b = (*SetProcessWindowStationFP) (h);
+               }
+               b = AllocConsole();
+               if (horig && h && h != horig && (*SetProcessWindowStationFP) (horig))
+               {
+                   (*CloseWindowStationFP) (h);
+               }
+           }
+           return b;
+       }
+   }
+   /* otherwise, fail */ 
+   return FALSE;
+}
+
+/* returns FALSE only on error conditions (not impl) */
+BOOL configure_startupinfo(STARTUPINFO* psi, BOOL bHaveInvisConsole,
+                           BOOL *bUsingPipes,
+                           HANDLE* hpToChild,  HANDLE* hpFromChild,
+                           HANDLE* hpToParent, HANDLE* hpFromParent)
+{
+    CONSOLE_CURSOR_INFO cci;
+    SECURITY_ATTRIBUTES handle_attrs;
+    HANDLE hpStdInput[2];
+    HANDLE hpStdOutput[2];
+
+    ZeroMemory (psi, sizeof (STARTUPINFO));
+    psi->cb = sizeof (STARTUPINFO);
+    psi->hStdInput   = GetStdHandle(STD_INPUT_HANDLE);
+    psi->hStdOutput  = GetStdHandle(STD_OUTPUT_HANDLE);
+    psi->hStdError   = GetStdHandle(STD_ERROR_HANDLE);
+    psi->dwFlags     = STARTF_USESHOWWINDOW | STARTF_USESTDHANDLES;
+    psi->wShowWindow = SW_HIDE;
+
+    /* foo() is some magic mechanism for determining that the HANDLEs 
+     * returned by GetStdHandle() are from a console, and not redirected
+     * or ptys of some sort.  If we have such a mechanism, then the 
+     * unconditional FreeConsole() at the top of setup_invisible_console()
+     * should be removed.
+     */
+/*
+    if (foo())
+    {
+       *bUsingPipes = FALSE;
+       return TRUE;
+    }
+*/
+
+    /* but for now, the only way we KNOW we have a console is
+     * if we created it ourselves
+     */
+    if (bHaveInvisConsole)
+    {
+       *bUsingPipes = FALSE;
+       return TRUE;
+    }
+
+    /* otherwise, set up pipes */
+    *bUsingPipes = TRUE;
+
+    handle_attrs.nLength = sizeof(SECURITY_ATTRIBUTES);
+    handle_attrs.bInheritHandle = TRUE;
+    handle_attrs.lpSecurityDescriptor = NULL;
+ 
+    /* create a pipe for child's stdin.  Don't allow child to */
+    /* inherit the write end of the pipe.                     */
+    CreatePipe (&hpStdInput[0], &hpStdInput[1], &handle_attrs, 0);
+    SetHandleInformation ( hpStdInput[1], HANDLE_FLAG_INHERIT, 0);
+ 
+    /* create a pipe for child's stdout.  Don't allow child to */
+    /* inherit the read end of the pipe.                       */
+    CreatePipe (&hpStdOutput[0], &hpStdOutput[1], &handle_attrs, 0);
+    SetHandleInformation ( hpStdOutput[0], HANDLE_FLAG_INHERIT, 0);
+
+    psi->hStdInput   = hpStdInput[0];
+    psi->hStdOutput  = hpStdOutput[1];
+    psi->hStdError   = hpStdOutput[1];
+
+    *hpToChild   = hpStdInput[1];
+    *hpFromChild = hpStdOutput[0];
+    *hpToParent  = hpStdOutput[1];
+    *hpFromParent= hpStdInput[0];
+
+    return TRUE;
+}
 int start_child(char* cmdline, int wait_for_child)
 {
    STARTUPINFO start;
-   SECURITY_ATTRIBUTES sec_attrs;
-   SECURITY_DESCRIPTOR sec_desc;
    PROCESS_INFORMATION child;
    DWORD retval;
+   BOOL bFuncReturn;
+   BOOL bHaveInvisConsole;
+   BOOL bUsingPipes;
+   HANDLE hToChild, hFromChild;
+   HANDLE hToParent, hFromParent;
 
    setup_win_environ();
 
-   memset (&start, 0, sizeof (start));
-   start.cb = sizeof (start);
-   start.dwFlags = STARTF_USESHOWWINDOW | STARTF_USESTDHANDLES;
-   start.wShowWindow = SW_HIDE;
-   start.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
-   start.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
-   start.hStdError = GetStdHandle(STD_ERROR_HANDLE);
-      
-   sec_attrs.nLength = sizeof (sec_attrs);
-   sec_attrs.lpSecurityDescriptor = NULL;
-   sec_attrs.bInheritHandle = FALSE;
+#ifdef DEBUG_FORCE_PIPES
+   bHaveInvisConsole = FALSE;
+   FreeConsole();
+#else
+   bHaveInvisConsole = setup_invisible_console();
+#endif
+
+   if (!configure_startupinfo(&start, bHaveInvisConsole,
+                              &bUsingPipes,
+                              &hToChild, &hFromChild, 
+                              &hToParent, &hFromParent))
+      error("could not start %s",cmdline);
+
+#ifdef DEBUG
+   message("Using Pipes: %s", (bUsingPipes ? "yes" : "no"));
+#endif
+
+   ZeroMemory( &child, sizeof(PROCESS_INFORMATION) );
 
-   if (CreateProcess (NULL, cmdline, &sec_attrs, NULL, TRUE, 0,
-                      NULL, NULL, &start, &child))
+   bFuncReturn = CreateProcess (NULL,
+       cmdline, /* command line                        */
+       NULL,    /* process security attributes         */
+       NULL,    /* primary thread security attributes  */
+       TRUE,    /* handles are inherited,              */
+       0,       /* creation flags                      */
+       NULL,    /* use parent's environment            */
+       NULL,    /* use parent's current directory      */
+       &start,  /* STARTUPINFO pointer                 */
+       &child); /* receives PROCESS_INFORMATION        */
+
+   if (bUsingPipes)
+   {
+       CloseHandle(hFromParent);
+       CloseHandle(hToParent);
+   }
+
+   if (bFuncReturn)
    {
       if (wait_for_child)
       {
-         WaitForSingleObject (child.hProcess, INFINITE);
-         GetExitCodeProcess (child.hProcess, &retval);
-      }
+          if (bUsingPipes)
+          {
+              HANDLE handles[2] = { child.hProcess, hFromChild };
+              COMMTIMEOUTS timeouts;
+    
+              /* only read bytes that are ready, and return immediately */
+              GetCommTimeouts(hFromChild, &timeouts);
+              timeouts.ReadIntervalTimeout = MAXDWORD;
+              timeouts.ReadTotalTimeoutMultiplier = 0;
+              timeouts.ReadTotalTimeoutConstant = 0;
+              SetCommTimeouts(hFromChild, &timeouts);
+    
+              while (WaitForMultipleObjects (2, handles, FALSE, INFINITE) == WAIT_OBJECT_0 + 1)
+              {
+                  char buffer[1024];
+                  DWORD dwRead;
+                  ReadFile (hFromChild, buffer, 1024, &dwRead, NULL);
+              }
+          }
+          else
+          {
+              WaitForSingleObject (child.hProcess, INFINITE);
+          }
+          GetExitCodeProcess (child.hProcess, &retval);
+      } 
       CloseHandle (child.hThread);
       CloseHandle (child.hProcess);
+      if (bUsingPipes)
+      {
+          CloseHandle (hFromChild);
+          CloseHandle (hToChild);
+      }
    }
    else
+   {
+      if (bUsingPipes)
+      {
+          CloseHandle (hFromChild);
+          CloseHandle (hToChild);
+      }
       error("could not start %s",cmdline);
+   }
    return retval;
 }
 void xemacs_special(char* exec)
@@ -353,7 +552,7 @@
     * STARTS WITH x:\ or x:/
     * execpath NOT used
     */
-    else if ((strlen(execname) > 3) && // avoid boundary errors
+    else if ((strlen(execname) > 3) && /* avoid boundary errors */
        (execname[1] == ':') &&
        ((execname[2] == '\\') || (execname[2] == '/')))
    {
@@ -459,10 +658,10 @@
                error("problem reading symbolic link for %s",exec_tmp);
             else
             {
-                // if realname starts with '/' it's a rootpath 
+                /* if realname starts with '/' it's a rootpath */
                 if (real_name[0] == '/')
                     strcpy(exec_tmp2,real_name);
-                else // otherwise, it's relative to the symlink's location
+                else /* otherwise, it's relative to the symlink's location */
                 {
                    CYGWIN_SPLIT_PATH((sym_link_name,exec_tmp2,dummy));
                    if (!endsWith(exec_tmp2,PATH_SEP_CHAR_STR))
@@ -495,8 +694,8 @@
     return retval;
 }void strip_exe(char* s)
 {
-   if ((strlen(s) > 4) && // long enough to have .exe extension
-       // second part not evaluated (short circuit) if exec_arg too short
+   if ((strlen(s) > 4) && /* long enough to have .exe extension */
+       /* second part not evaluated (short circuit) if exec_arg too short */
        (stricmp(&(s[strlen(s)-4]),".exe") == 0))
       s[strlen(s)-4] = '\0';
 }
@@ -557,12 +756,13 @@
       error("internal error - my own name has no path\n%s",modname);
    tmp_execname = p + 1;
    p[0] = '\0';
-   // if invoked by a name like "runxemacs" then strip off
-   // the "run" and let "xemacs" be execname.
-   // To check for this, make that:
-   //   1) first three chars are "run"
-   //   2) but the string doesn't end there, or start ".exe"
-   // Also, set "compact_invocation" TRUE
+   /* if invoked by a name like "runxemacs" then strip off
+    * the "run" and let "xemacs" be execname.
+    * To check for this, make that:
+    *   1) first three chars are "run"
+    *   2) but the string doesn't end there, or start ".exe"
+    * Also, set "compact_invocation" TRUE
+    */
    if ( ((tmp_execname[0] == 'r') || (tmp_execname[0] == 'R')) &&
         ((tmp_execname[1] == 'u') || (tmp_execname[1] == 'U')) &&
         ((tmp_execname[2] == 'n') || (tmp_execname[2] == 'N')) &&

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Problem reports:       http://cygwin.com/problems.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]