Changeset 3447 in kBuild

Timestamp:

Sep 11, 2020 1:22:14 PM (5 years ago)

Author:

bird

Message:

kash: Avoid handle inheritance on windows when possible, allowing concurrent CreateProcess calls.

Location:

trunk/src/kash

Files:

• shfile.c (modified) (4 diffs)
• shfile.h (modified) (1 diff)
• shinstance.c (modified) (10 diffs)
• shinstance.h (modified) (1 diff)

trunk/src/kash/shfile.c

@@ -1050 +1050 @@
 # endif /* SH_FORKED_MODE */
 
+/** shfile_exec_win helper that make sure there are no _O_APPEND handles. */
+static KBOOL shfile_exec_win_no_append(shfdtab *pfdtab, unsigned count)
+{
+    unsigned i;
+    for (i = 0; i < count; i++)
+        if (   (pfdtab->tab[i].oflags & _O_APPEND)
+            && (pfdtab->tab[i].oflags & (_O_WRONLY | _O_RDWR)))
+            return K_FALSE;
+    return K_TRUE;
+}
+
 /**
  * Helper for sh_execve.
@@ -1058 +1069 @@
  * it will restore the handle inheritability properties.
  *
- * @returns Pointer to CRT data if prepare is 1, NULL if prepare is 0.
+ * @returns 0 on success, non-zero on failure.
  * @param   pfdtab  The file descriptor table.
  * @param   prepare Which call, 1 if before, 0 if after and success, -1 if after on failure.
- * @param   sizep   Where to store the size of the returned data.
- * @param   hndls   Where to store the three standard handles.
- */
-void *shfile_exec_win(shfdtab *pfdtab, int prepare, unsigned short *sizep, intptr_t *hndls)
-{
-    void       *pvRet;
-    shmtxtmp    tmp;
-    unsigned    count;
-    unsigned    i;
+ * @param   info    The info structure.
+ */
+int shfile_exec_win(shfdtab *pfdtab, int prepare, shfdexecwin *info)
+{
+    STARTUPINFOA   *strtinfo = (STARTUPINFOA *)info->strtinfo;
+    int             rc = 0;
+    shmtxtmp        tmp;
+    unsigned        count;
+    unsigned        i;
 
     shmtx_enter(&pfdtab->mtx, &tmp);
@@ -1077 +1088 @@
            ? pfdtab->size
            : (0x10000-4) / (1 + sizeof(HANDLE));
-    while (count > 3 && pfdtab->tab[count - 1].fd == -1)
+    while (   count > 3
+           && (   pfdtab->tab[count - 1].fd == -1
+               || (pfdtab->tab[count - 1].shflags & SHFILE_FLAGS_CLOSE_ON_EXEC)))
         count--;
 
     if (prepare > 0)
     {
-        size_t      cbData = sizeof(int) + count * (1 + sizeof(HANDLE));
-        uint8_t    *pbData = sh_malloc(shthread_get_shell(), cbData);
-        uint8_t    *paf = pbData + sizeof(int);
-        HANDLE     *pah = (HANDLE *)(paf + count);
-
-        *(int *)pbData = count;
-
-        i = count;
-        while (i-- > 0)
-        {
-            if (    pfdtab->tab[i].fd == i
-                &&  !(pfdtab->tab[i].shflags & SHFILE_FLAGS_CLOSE_ON_EXEC))
+        if (count <= 3 && shfile_exec_win_no_append(pfdtab, count))
+        {
+            info->inherithandles  = 0;
+            info->startsuspended  = 1;
+            strtinfo->cbReserved2 = 0;
+            strtinfo->lpReserved2 = NULL;
+        }
+        else
+        {
+            size_t      cbData = sizeof(int) + count * (1 + sizeof(HANDLE));
+            uint8_t    *pbData = sh_malloc(shthread_get_shell(), cbData);
+            uint8_t    *paf = pbData + sizeof(int);
+            HANDLE     *pah = (HANDLE *)(paf + count);
+
+            info->inherithandles  = 1;
+            info->startsuspended  = 0;
+            strtinfo->cbReserved2 = (unsigned short)cbData;
+            strtinfo->lpReserved2 = pbData;
+
+            shmtx_leave(&pfdtab->mtx, &tmp); /* should be harmless as this isn't really necessary at all. */
+            shmtx_enter(&g_sh_exec_inherit_mtx, &info->tmp);
+            shmtx_enter(&pfdtab->mtx, &tmp);
+
+            *(int *)pbData = count;
+
+            i = count;
+            while (i-- > 0)
             {
-                HANDLE hFile = shfile_set_inherit_win(&pfdtab->tab[i], 1);
-                TRACE2((NULL, "  #%d: native=%#x oflags=%#x shflags=%#x\n",
-                        i, hFile, pfdtab->tab[i].oflags, pfdtab->tab[i].shflags));
-                paf[i] = FOPEN;
-                if (pfdtab->tab[i].oflags & _O_APPEND)
-                    paf[i] |= FAPPEND;
-                if (pfdtab->tab[i].oflags & _O_TEXT)
-                    paf[i] |= FTEXT;
-                switch (pfdtab->tab[i].shflags & SHFILE_FLAGS_TYPE_MASK)
+                if (    pfdtab->tab[i].fd == i
+                    &&  !(pfdtab->tab[i].shflags & SHFILE_FLAGS_CLOSE_ON_EXEC))
                 {
-                    case SHFILE_FLAGS_TTY:  paf[i] |= FDEV; break;
-                    case SHFILE_FLAGS_PIPE: paf[i] |= FPIPE; break;
+                    HANDLE hFile = shfile_set_inherit_win(&pfdtab->tab[i], 1);
+                    TRACE2((NULL, "  #%d: native=%#x oflags=%#x shflags=%#x\n",
+                            i, hFile, pfdtab->tab[i].oflags, pfdtab->tab[i].shflags));
+                    paf[i] = FOPEN;
+                    if (pfdtab->tab[i].oflags & _O_APPEND)
+                        paf[i] |= FAPPEND;
+                    if (pfdtab->tab[i].oflags & _O_TEXT)
+                        paf[i] |= FTEXT;
+                    switch (pfdtab->tab[i].shflags & SHFILE_FLAGS_TYPE_MASK)
+                    {
+                        case SHFILE_FLAGS_TTY:  paf[i] |= FDEV; break;
+                        case SHFILE_FLAGS_PIPE: paf[i] |= FPIPE; break;
+                    }
+                    pah[i] = hFile;
                 }
-                pah[i] = hFile;
+                else
+                {
+                    paf[i] = 0;
+                    pah[i] = INVALID_HANDLE_VALUE;
+                }
+            }
+        }
+
+        for (i = 0; i < 3; i++)
+        {
+            if (    i < count
+                &&  pfdtab->tab[i].fd == i)
+            {
+                info->replacehandles[i] = 1;
+                info->handles[i] = pfdtab->tab[i].native;
             }
             else
             {
-                paf[i] = 0;
-                pah[i] = INVALID_HANDLE_VALUE;
+                info->replacehandles[i] = 0;
+                info->handles[i] = (intptr_t)INVALID_HANDLE_VALUE;
             }
-        }
-
-        for (i = 0; i < 3; i++)
-        {
-            if (    i < count
-                &&  pfdtab->tab[i].fd == i)
-                hndls[i] = pfdtab->tab[i].native;
-            else
-                hndls[i] = (intptr_t)INVALID_HANDLE_VALUE;
             TRACE2((NULL, "shfile_exec_win: i=%d count=%d fd=%d native=%d hndls[%d]=\n",
-                    i, count, pfdtab->tab[i].fd, pfdtab->tab[i].native, i, hndls[i]));
-        }
-
-        *sizep = (unsigned short)cbData;
-        pvRet = pbData;
+                    i, count, pfdtab->tab[i].fd, pfdtab->tab[i].native, i, info->handles[i]));
+        }
     }
     else
     {
         shfile *file = pfdtab->tab;
-        assert(!hndls);
-        assert(!sizep);
+
+        sh_free(NULL, strtinfo->lpReserved2);
+        strtinfo->lpReserved2 = NULL;
+
         i = count;
         if (prepare == 0)
@@ -1151 +1188 @@
                 }
             }
-        else
+        else if (info->inherithandles)
             for (i = 0; i < count; i++, file++)
                 if (    file->fd == i
                     &&  !(file->shflags & SHFILE_FLAGS_CLOSE_ON_EXEC))
                     shfile_set_inherit_win(file, 0);
-        pvRet = NULL;
+
+        if (info->inherithandles)
+            shmtx_leave(&g_sh_exec_inherit_mtx, &info->tmp);
     }
 
     shmtx_leave(&pfdtab->mtx, &tmp);
-    return pvRet;
+    return rc;
 }
 

trunk/src/kash/shfile.h

@@ -134 +134 @@
 void shfile_uninit(shfdtab *);
 void shfile_fork_win(shfdtab *pfdtab, int set, intptr_t *hndls);
-void *shfile_exec_win(shfdtab *pfdtab, int prepare, unsigned short *sizep, intptr_t *hndls);
+typedef struct shfdexecwin
+{
+    int inherithandles;
+    int startsuspended;
+    shmtxtmp tmp;
+    int replacehandles[3];
+    intptr_t handles[3];
+    void *strtinfo;
+} shfdexecwin;
+int shfile_exec_win(shfdtab *pfdtab, int prepare, shfdexecwin *info);
 int shfile_exec_unix(shfdtab *pfdtab);
 

trunk/src/kash/shinstance.c

@@ -50 +50 @@
 #if K_OS == K_OS_WINDOWS
 # include <Windows.h>
+# include "nt/nt_child_inject_standard_handles.h"
 # ifdef SH_FORKED_MODE
 extern pid_t shfork_do(shinstance *psh); /* shforkA-win.asm */
@@ -70 +71 @@
 #ifndef SH_FORKED_MODE
 /** Mutex serializing exec/spawn to prevent unwanted file inherting. */
-static shmtx        g_sh_exec_mtx;
+shmtx               g_sh_exec_inherit_mtx;
 #endif
 /** The mutex protecting the the globals and some shell instance members (sigs). */
@@ -82 +83 @@
 /** The number of shells. */
 static int volatile g_num_shells;
+/* Statistics: Number of subshells spawned. */
+static KU64             g_stat_subshells = 0;
+/* Statistics: Number of program exec'ed. */
+static KU64 volatile    g_stat_execs = 0;
+#if K_OS == K_OS_WINDOWS
+/* Statistics: Number of serialized exec calls. */
+static KU64 volatile    g_stat_execs_serialized = 0;
+#endif
 /** Per signal state for determining a common denominator.
  * @remarks defaults and unmasked actions aren't counted. */
@@ -172 +181 @@
     if (psh->rootshell)
         g_sh_root = psh;
+    else
+        g_stat_subshells++;
 
     psh->next = NULL;
@@ -637 +648 @@
     shmtx_init(&g_sh_mtx);
 #ifndef SH_FORKED_MODE
-    shmtx_init(&g_sh_exec_mtx);
+    shmtx_init(&g_sh_exec_inherit_mtx);
 #endif
 
@@ -1660 +1671 @@
     int rc;
 
+    g_stat_execs++;
+
 #ifdef DEBUG
     /* log it all */
@@ -1698 +1711 @@
          * This ain't quite straight forward on Windows...
          */
-#ifndef SH_FORKED_MODE
-        shmtxtmp tmp;
-#endif
         PROCESS_INFORMATION ProcInfo;
         STARTUPINFO StrtInfo;
-        intptr_t hndls[3];
+        shfdexecwin fdinfo;
         char *cwd = shfile_getcwd(&psh->fdtab, NULL, 0);
         char *cmdline;
@@ -1812 +1822 @@
 
         /* File handles. */
-#ifndef SH_FORKED_MODE
-        shmtx_enter(&g_sh_exec_mtx, &tmp);
-#endif
-        StrtInfo.dwFlags   |= STARTF_USESTDHANDLES;
-        StrtInfo.lpReserved2 = shfile_exec_win(&psh->fdtab, 1 /* prepare */, &StrtInfo.cbReserved2, hndls);
-        StrtInfo.hStdInput  = (HANDLE)hndls[0];
-        StrtInfo.hStdOutput = (HANDLE)hndls[1];
-        StrtInfo.hStdError  = (HANDLE)hndls[2];
+        fdinfo.strtinfo = &StrtInfo;
+        shfile_exec_win(&psh->fdtab, 1 /* prepare */, &fdinfo);
+        TRACE2((psh, "sh_execve: inherithandles=%d replacehandles={%d,%d,%d} handles={%p,%p,%p} suspended=%d Reserved2=%p LB %#x\n",
+                fdinfo.inherithandles, fdinfo.replacehandles[0], fdinfo.replacehandles[1], fdinfo.replacehandles[2],
+                fdinfo.handles[0], fdinfo.handles[1], fdinfo.handles[3], fdinfo.startsuspended,
+                StrtInfo.lpReserved2, StrtInfo.cbReserved2));
+        if (!fdinfo.inherithandles)
+        {
+            StrtInfo.dwFlags |= STARTF_USESTDHANDLES;
+            StrtInfo.hStdInput  = INVALID_HANDLE_VALUE;
+            StrtInfo.hStdOutput = INVALID_HANDLE_VALUE;
+            StrtInfo.hStdError  = INVALID_HANDLE_VALUE;
+        }
+        else
+        {
+            StrtInfo.dwFlags |= STARTF_USESTDHANDLES;
+            StrtInfo.hStdInput  = (HANDLE)fdinfo.handles[0];
+            StrtInfo.hStdOutput = (HANDLE)fdinfo.handles[1];
+            StrtInfo.hStdError  = (HANDLE)fdinfo.handles[2];
+            g_stat_execs_serialized++;
+        }
 
         /* Get going... */
-        rc = CreateProcess(exe,
-                           cmdline,
-                           NULL,         /* pProcessAttributes */
-                           NULL,         /* pThreadAttributes */
-                           TRUE,         /* bInheritHandles */
-                           0,            /* dwCreationFlags */
-                           envblock,
-                           cwd,
-                           &StrtInfo,
-                           &ProcInfo);
-
-        shfile_exec_win(&psh->fdtab, rc ? 0 /* done */ : -1 /* done but failed */, NULL, NULL);
-#ifndef SH_FORKED_MODE
-        shmtx_leave(&g_sh_exec_mtx, &tmp);
-#endif
+        rc = CreateProcessA(exe,
+                            cmdline,
+                            NULL,         /* pProcessAttributes */
+                            NULL,         /* pThreadAttributes */
+                            fdinfo.inherithandles,
+                            fdinfo.startsuspended ? CREATE_SUSPENDED : 0,
+                            envblock,
+                            cwd,
+                            &StrtInfo,
+                            &ProcInfo);
         if (rc)
         {
@@ -1842 +1860 @@
             DWORD dwExitCode;
 
+            if (fdinfo.startsuspended)
+            {
+                char errmsg[512];
+                rc = nt_child_inject_standard_handles(ProcInfo.hProcess, fdinfo.replacehandles, (HANDLE*)&fdinfo.handles[0],
+                                                      errmsg, sizeof(errmsg));
+                if (!rc)
+                {
+                    rc = ResumeThread(ProcInfo.hThread);
+                    if (!rc)
+                        TRACE2((psh, "sh_execve: ResumeThread failed: %u -> errno=ENXIO\n", GetLastError()));
+                }
+                else
+                {
+                    TRACE2((psh, "sh_execve: nt_child_inject_standard_handles failed: %d -> errno=ENXIO; %s\n", rc, errmsg));
+                    rc = FALSE;
+                }
+                errno = ENXIO;
+            }
+
+            shfile_exec_win(&psh->fdtab, rc ? 0 /* done */ : -1 /* done but failed */, &fdinfo);
+
             CloseHandle(ProcInfo.hThread);
-            dwErr = WaitForSingleObject(ProcInfo.hProcess, INFINITE);
-            assert(dwErr == WAIT_OBJECT_0);
-
-            if (GetExitCodeProcess(ProcInfo.hProcess, &dwExitCode))
+            ProcInfo.hThread = INVALID_HANDLE_VALUE;
+            if (rc)
             {
-                CloseHandle(ProcInfo.hProcess);
-                sh__exit(psh, dwExitCode);
+                /*
+                 * Wait for it and forward the exit code.
+                 */
+                dwErr = WaitForSingleObject(ProcInfo.hProcess, INFINITE);
+                assert(dwErr == WAIT_OBJECT_0);
+
+                if (GetExitCodeProcess(ProcInfo.hProcess, &dwExitCode))
+                {
+#ifndef SH_FORKED_MODE
+                    /** @todo signal the end of this subshell now, we can do the cleaning up
+                     *        after the parent shell has resumed. */
+#endif
+                    CloseHandle(ProcInfo.hProcess);
+                    ProcInfo.hProcess = INVALID_HANDLE_VALUE;
+                    sh__exit(psh, dwExitCode);
+                }
+
+                /* this shouldn't happen... */
+                TRACE2((psh, "sh_execve: GetExitCodeProcess failed: %u\n", GetLastError()));
+                assert(0);
+                errno = EINVAL;
             }
-            TRACE2((psh, "sh_execve: GetExitCodeProcess failed: %u\n", GetLastError()));
-            assert(0);
+            TerminateProcess(ProcInfo.hProcess, 0x40000015);
             CloseHandle(ProcInfo.hProcess);
-            errno = EINVAL;
         }
         else
         {
             DWORD dwErr = GetLastError();
+
+            shfile_exec_win(&psh->fdtab, -1 /* done but failed */, &fdinfo);
+
             switch (dwErr)
             {
@@ -1865 +1922 @@
                 case ERROR_BAD_EXE_FORMAT:          errno = ENOEXEC; break;
                 case ERROR_INVALID_EXE_SIGNATURE:   errno = ENOEXEC; break;
-                default:
-                    errno = EINVAL;
-                    break;
+                default:                            errno = EINVAL; break;
             }
             TRACE2((psh, "sh_execve: dwErr=%d -> errno=%d\n", dwErr, errno));
         }
-
     }
     rc = -1;

trunk/src/kash/shinstance.h

@@ -59 +59 @@
 # define strncasecmp strnicmp
 #endif
+
+extern shmtx g_sh_exec_inherit_mtx;
 
 /**