Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r?mhowell draft
authorKris Maglione <maglione.k@gmail.com>
Fri, 27 May 2016 16:15:39 -0700
changeset 372325 608ee4995defd511e845b0345a714e6dcfa2bd37
parent 372309 1bd815acf6d3d51ded73a0ffbe05bd8a2c515784
child 522166 0766d1149bd607ebad136fa3dafbc971d173c783
push id19510
push usermaglione.k@gmail.com
push dateSat, 28 May 2016 02:22:36 +0000
reviewersmhowell
bugs1276386
milestone49.0a1
Bug 1276386 - Prevent processes from inheriting extra file descriptors on Windows. r?mhowell MozReview-Commit-ID: IFi2Z7sqaxW
toolkit/modules/subprocess/subprocess_shared_win.js
toolkit/modules/subprocess/subprocess_worker_win.js
toolkit/modules/subprocess/test/xpcshell/test_subprocess.js
--- a/toolkit/modules/subprocess/subprocess_shared_win.js
+++ b/toolkit/modules/subprocess/subprocess_shared_win.js
@@ -8,20 +8,22 @@
 /* exported LIBC, Win, createPipe, libc */
 
 const LIBC = OS.Constants.libc;
 
 const Win = OS.Constants.Win;
 
 const LIBC_CHOICES = ["kernel32.dll"];
 
-const win32 = {
+var win32 = {
   // On Windows 64, winapi_abi is an alias for default_abi.
   WINAPI: ctypes.winapi_abi,
 
+  VOID: ctypes.void_t,
+
   BYTE: ctypes.uint8_t,
   WORD: ctypes.uint16_t,
   DWORD: ctypes.uint32_t,
 
   UINT: ctypes.unsigned_int,
   UCHAR: ctypes.unsigned_char,
 
   BOOL: ctypes.bool,
@@ -29,59 +31,72 @@ const win32 = {
   HANDLE: ctypes.voidptr_t,
   PVOID: ctypes.voidptr_t,
   LPVOID: ctypes.voidptr_t,
 
   CHAR: ctypes.char,
   WCHAR: ctypes.jschar,
 
   ULONG_PTR: ctypes.uintptr_t,
+
+  SIZE_T: ctypes.size_t,
+  PSIZE_T: ctypes.size_t.ptr,
 };
 
 Object.assign(win32, {
+  DWORD_PTR: win32.ULONG_PTR,
+
   LPSTR: win32.CHAR.ptr,
   LPWSTR: win32.WCHAR.ptr,
 
   LPBYTE: win32.BYTE.ptr,
   LPDWORD: win32.DWORD.ptr,
   LPHANDLE: win32.HANDLE.ptr,
+
+  // This is an opaque type.
+  PROC_THREAD_ATTRIBUTE_LIST: ctypes.char.array(),
+  LPPROC_THREAD_ATTRIBUTE_LIST: ctypes.char.ptr,
 });
 
 Object.assign(win32, {
   LPCSTR: win32.LPSTR,
   LPCWSTR: win32.LPWSTR,
   LPCVOID: win32.LPVOID,
 });
 
 Object.assign(win32, {
   CREATE_NEW_CONSOLE: 0x00000010,
   CREATE_UNICODE_ENVIRONMENT: 0x00000400,
+  EXTENDED_STARTUPINFO_PRESENT: 0x00080000,
   CREATE_NO_WINDOW: 0x08000000,
 
   STARTF_USESTDHANDLES: 0x0100,
 
   DUPLICATE_CLOSE_SOURCE: 0x01,
   DUPLICATE_SAME_ACCESS: 0x02,
 
   ERROR_HANDLE_EOF: 38,
   ERROR_BROKEN_PIPE: 109,
+  ERROR_INSUFFICIENT_BUFFER: 122,
 
   FILE_FLAG_OVERLAPPED: 0x40000000,
 
   PIPE_TYPE_BYTE: 0x00,
 
   PIPE_ACCESS_INBOUND: 0x01,
   PIPE_ACCESS_OUTBOUND: 0x02,
   PIPE_ACCESS_DUPLEX: 0x03,
 
   PIPE_WAIT: 0x00,
   PIPE_NOWAIT: 0x01,
 
   STILL_ACTIVE: 259,
 
+  PROC_THREAD_ATTRIBUTE_HANDLE_LIST: 0x00020002,
+
   // These constants are 32-bit unsigned integers, but Windows defines
   // them as negative integers cast to an unsigned type.
   STD_INPUT_HANDLE: -10 + 0x100000000,
   STD_OUTPUT_HANDLE: -11 + 0x100000000,
   STD_ERROR_HANDLE: -12 + 0x100000000,
 
   WAIT_TIMEOUT: 0x00000102,
   WAIT_FAILED: 0xffffffff,
@@ -126,16 +141,24 @@ Object.assign(win32, {
     {"cbReserved2": win32.WORD},
     {"lpReserved2": win32.LPBYTE},
     {"hStdInput": win32.HANDLE},
     {"hStdOutput": win32.HANDLE},
     {"hStdError": win32.HANDLE},
   ]),
 });
 
+Object.assign(win32, {
+  STARTUPINFOEXW: new ctypes.StructType("STARTUPINFOEXW", [
+    {"StartupInfo": win32.STARTUPINFOW},
+    {"lpAttributeList": win32.LPPROC_THREAD_ATTRIBUTE_LIST},
+  ]),
+});
+
+
 var libc = new Library("libc", LIBC_CHOICES, {
   CloseHandle: [
     win32.WINAPI,
     win32.BOOL,
     win32.HANDLE, /* hObject */
   ],
 
   CreateEventW: [
@@ -191,16 +214,22 @@ var libc = new Library("libc", LIBC_CHOI
     win32.BOOL, /* bInheritHandle */
     win32.DWORD, /* dwCreationFlags */
     win32.LPVOID, /* opt lpEnvironment */
     win32.LPCWSTR, /* opt lpCurrentDirectory */
     win32.STARTUPINFOW.ptr, /* lpStartupInfo */
     win32.PROCESS_INFORMATION.ptr, /* out lpProcessInformation */
   ],
 
+  DeleteProcThreadAttributeList: [
+    win32.WINAPI,
+    win32.VOID,
+    win32.LPPROC_THREAD_ATTRIBUTE_LIST, /* in/out lpAttributeList */
+  ],
+
   DuplicateHandle: [
     win32.WINAPI,
     win32.BOOL,
     win32.HANDLE, /* hSourceProcessHandle */
     win32.HANDLE, /* hSourceHandle */
     win32.HANDLE, /* hTargetProcessHandle */
     win32.LPHANDLE, /* out lpTargetHandle */
     win32.DWORD, /* dwDesiredAccess */
@@ -231,36 +260,40 @@ var libc = new Library("libc", LIBC_CHOI
 
   GetExitCodeProcess: [
     win32.WINAPI,
     win32.BOOL,
     win32.HANDLE, /* hProcess */
     win32.LPDWORD, /* lpExitCode */
   ],
 
-  GetLastError: [
-    win32.WINAPI,
-    win32.DWORD,
-  ],
-
   GetOverlappedResult: [
     win32.WINAPI,
     win32.BOOL,
     win32.HANDLE, /* hFile */
     win32.OVERLAPPED.ptr, /* lpOverlapped */
     win32.LPDWORD, /* lpNumberOfBytesTransferred */
     win32.BOOL, /* bWait */
   ],
 
   GetStdHandle: [
     win32.WINAPI,
     win32.HANDLE,
     win32.DWORD, /* nStdHandle */
   ],
 
+  InitializeProcThreadAttributeList: [
+    win32.WINAPI,
+    win32.BOOL,
+    win32.LPPROC_THREAD_ATTRIBUTE_LIST, /* out opt lpAttributeList */
+    win32.DWORD, /* dwAttributeCount */
+    win32.DWORD, /* dwFlags */
+    win32.PSIZE_T, /* in/out lpSize */
+  ],
+
   ReadFile: [
     win32.WINAPI,
     win32.BOOL,
     win32.HANDLE, /* hFile */
     win32.LPVOID, /* out lpBuffer */
     win32.DWORD, /* nNumberOfBytesToRead */
     win32.LPDWORD, /* opt out lpNumberOfBytesRead */
     win32.OVERLAPPED.ptr, /* opt in/out lpOverlapped */
@@ -268,16 +301,28 @@ var libc = new Library("libc", LIBC_CHOI
 
   TerminateProcess: [
     win32.WINAPI,
     win32.BOOL,
     win32.HANDLE, /* hProcess */
     win32.UINT, /* uExitCode */
   ],
 
+  UpdateProcThreadAttribute: [
+    win32.WINAPI,
+    win32.BOOL,
+    win32.LPPROC_THREAD_ATTRIBUTE_LIST, /* in/out lpAttributeList */
+    win32.DWORD, /* dwFlags */
+    win32.DWORD_PTR, /* Attribute */
+    win32.PVOID, /* lpValue */
+    win32.SIZE_T, /* cbSize */
+    win32.PVOID, /* out opt lpPreviousValue */
+    win32.PSIZE_T, /* opt lpReturnSize */
+  ],
+
   WaitForMultipleObjects: [
     win32.WINAPI,
     win32.DWORD,
     win32.DWORD, /* nCount */
     win32.HANDLE.ptr, /* hHandles */
     win32.BOOL, /* bWaitAll */
     win32.DWORD, /* dwMilliseconds */
   ],
@@ -341,8 +386,43 @@ win32.createPipe = function(secAttr, rea
   if (isInvalid(writeHandle)) {
     libc.CloseHandle(readHandle);
     return [];
   }
 
   return [win32.Handle(readHandle),
           win32.Handle(writeHandle)];
 };
+
+
+win32.createThreadAttributeList = function(handles) {
+  try {
+    libc.InitializeProcThreadAttributeList;
+    libc.DeleteProcThreadAttributeList;
+    libc.UpdateProcThreadAttribute;
+  } catch (e) {
+    // This is only supported in Windows Vista and later.
+    return null;
+  }
+
+  let size = win32.SIZE_T();
+  if (!libc.InitializeProcThreadAttributeList(null, 1, 0, size.address()) &&
+      ctypes.winLastError != win32.ERROR_INSUFFICIENT_BUFFER) {
+    return null;
+  }
+
+  let attrList = win32.PROC_THREAD_ATTRIBUTE_LIST(size.value);
+
+  if (!libc.InitializeProcThreadAttributeList(attrList, 1, 0, size.address())) {
+    return null;
+  }
+
+  let ok = libc.UpdateProcThreadAttribute(
+    attrList, 0, win32.PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
+    handles, handles.constructor.size, null, null);
+
+  if (!ok) {
+    libc.DeleteProcThreadAttributeList(attrList);
+    return null;
+  }
+
+  return attrList;
+}
--- a/toolkit/modules/subprocess/subprocess_worker_win.js
+++ b/toolkit/modules/subprocess/subprocess_worker_win.js
@@ -167,17 +167,17 @@ class InputPipe extends Pipe {
    *        The number of bytes to read.
    */
   readBuffer(count) {
     this.buffer = new ArrayBuffer(count);
 
     let ok = libc.ReadFile(this.handle, this.buffer, count,
                            null, this.overlapped.address());
 
-    if (!ok && (!this.process.handle || libc.GetLastError())) {
+    if (!ok && (!this.process.handle || libc.winLastError)) {
       this.onError();
     } else {
       io.updatePollEvents();
     }
   }
 
   /**
    * Called when our pending overlapped IO operation has completed, whether
@@ -256,17 +256,17 @@ class OutputPipe extends Pipe {
    *        The buffer to write.
    */
   writeBuffer(buffer) {
     this.buffer = buffer;
 
     let ok = libc.WriteFile(this.handle, buffer, buffer.byteLength,
                             null, this.overlapped.address());
 
-    if (!ok && libc.GetLastError()) {
+    if (!ok && libc.winLastError) {
       this.onError();
     } else {
       io.updatePollEvents();
     }
   }
 
   /**
    * Called when our pending overlapped IO operation has completed, whether
@@ -420,39 +420,58 @@ class Process extends BaseProcess {
 
     let envp = this.stringList(options.environment);
 
     let handles = this.initPipes(options);
 
     let processFlags = win32.CREATE_NO_WINDOW
                      | win32.CREATE_UNICODE_ENVIRONMENT;
 
-    let startupInfo = new win32.STARTUPINFOW();
+    let startupInfoEx = new win32.STARTUPINFOEXW();
+    let startupInfo = startupInfoEx.StartupInfo;
+
     startupInfo.cb = win32.STARTUPINFOW.size;
     startupInfo.dwFlags = win32.STARTF_USESTDHANDLES;
 
     startupInfo.hStdInput = handles[0];
     startupInfo.hStdOutput = handles[1];
     startupInfo.hStdError = handles[2];
 
+    // Note: This needs to be kept alive until we destroy the attribute list.
+    let handleArray = win32.HANDLE.array()(handles)
+
+    let threadAttrs = win32.createThreadAttributeList(handleArray);
+    if (threadAttrs) {
+      // If have thread attributes to pass, pass the size of the full extended
+      // startup info struct.
+      processFlags |= win32.EXTENDED_STARTUPINFO_PRESENT,
+      startupInfo.cb = win32.STARTUPINFOEXW.size;
+
+      startupInfoEx.lpAttributeList = threadAttrs;
+    }
+
     let procInfo = new win32.PROCESS_INFORMATION();
 
     let ok = libc.CreateProcessW(
       command, args.join(" "),
       null, /* Security attributes */
       null, /* Thread security attributes */
       true, /* Inherits handles */
       processFlags, envp, options.workdir,
       startupInfo.address(),
       procInfo.address());
 
     for (let handle of new Set(handles)) {
       handle.dispose();
     }
 
+    if (threadAttrs) {
+      libc.DeleteProcThreadAttributeList(threadAttrs);
+    }
+
     if (!ok) {
       for (let pipe of this.pipes) {
         pipe.close();
       }
       throw new Error("Failed to create process");
     }
 
     libc.CloseHandle(procInfo.hThread);
--- a/toolkit/modules/subprocess/test/xpcshell/test_subprocess.js
+++ b/toolkit/modules/subprocess/test/xpcshell/test_subprocess.js
@@ -408,16 +408,54 @@ add_task(function* test_subprocess_inval
     "Promise should be rejected on EOF");
 
   let {exitCode} = yield proc.wait();
 
   equal(exitCode, 0, "Got expected exit code");
 });
 
 
+if (AppConstants.isPlatformAndVersionAtLeast("win", "6")) {
+  add_task(function* test_subprocess_inherited_descriptors() {
+    let {ctypes, libc, win32} = Cu.import("resource://gre/modules/subprocess/subprocess_win.jsm");
+
+    let secAttr = new win32.SECURITY_ATTRIBUTES();
+    secAttr.nLength = win32.SECURITY_ATTRIBUTES.size;
+    secAttr.bInheritHandle = true;
+
+    let handles = win32.createPipe(secAttr, 0);
+
+
+    let proc = yield Subprocess.call({
+      command: PYTHON,
+      arguments: ["-u", TEST_SCRIPT, "echo"],
+    });
+
+
+    // Close the output end of the pipe.
+    // Ours should be the only copy, so reads should fail after this.
+    handles[1].dispose();
+
+    let buffer = new ArrayBuffer(1);
+    let succeeded = libc.ReadFile(handles[0], buffer, buffer.byteLength,
+                                  null, null);
+
+    ok(!succeeded, "ReadFile should fail on broken pipe");
+    equal(ctypes.winLastError, win32.ERROR_BROKEN_PIPE, "Read should fail with ERROR_BROKEN_PIPE");
+
+
+    proc.stdin.close();
+
+    let {exitCode} = yield proc.wait();
+
+    equal(exitCode, 0, "Got expected exit code");
+  });
+}
+
+
 add_task(function* test_subprocess_wait() {
   let proc = yield Subprocess.call({
     command: PYTHON,
     arguments: ["-u", TEST_SCRIPT, "exit", "42"],
   });
 
   let {exitCode} = yield proc.wait();