Windows,JNI: check cmd length for CreateProcess

Error out if the command we try to pass to
CreateProcess is longer than the limit.

Doing so results in a nicer error message than
"The parameter is incorrect" which is confusing.

In this commit I also improve the error reporting
of CreateProcessWithExplicitHandles.

See https://github.com/bazelbuild/bazel/issues/4083
See https://github.com/bazelbuild/bazel/issues/4096

Change-Id: I00ec52238706fd8140483eddb488c3069eaa7814
PiperOrigin-RevId: 175969789

1 files changed, 100 insertions, 66 deletions

diff --git a/src/main/native/windows/processes-jni.cc b/src/main/native/windows/processes-jni.cc
index 27d8b4548d..eb6655cef7 100644
--- a/src/main/native/windows/processes-jni.cc
+++ b/src/main/native/windows/processes-jni.cc
@@ -14,6 +14,7 @@
 
 #define WIN32_LEAN_AND_MEAN
 
+#include <wchar.h>
 #include <windows.h>
 
 #include <atomic>
@@ -26,6 +27,10 @@
 #include "src/main/native/windows/jni-util.h"
 #include "src/main/native/windows/util.h"
 
+// Maximum command line length is 2^15 characters including the null terminator.
+// https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx
+static const size_t MAX_CMDLINE = 1 << 15;
+
 template <typename T>
 static std::wstring ToString(const T& e) {
   std::wstringstream s;
@@ -133,71 +138,109 @@ static_assert(sizeof(jchar) == sizeof(WCHAR),
 
 static jlong PtrAsJlong(void* p) { return reinterpret_cast<jlong>(p); }
 
-// The following CreateProcessWithExplicitHandles function is from oldnewthing.
+// The following CreateProcessWithExplicitHandles function is based on an
+// implementation of the same function in the following OldNewThing blog post:
 // https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873
-// We need it to prevent unintended handles being inherited by child process,
-// see http://support.microsoft.com/kb/315939
-static BOOL CreateProcessWithExplicitHandles(
-    /* __in_opt    */ LPCWSTR lpApplicationName,
+// We need this function to prevent the child process from inheriting unintended
+// handles. See http://support.microsoft.com/kb/315939
+static std::wstring CreateProcessWithExplicitHandles(
     /* __inout_opt */ LPWSTR lpCommandLine,
-    /* __in_opt    */ LPSECURITY_ATTRIBUTES lpProcessAttributes,
-    /* __in_opt    */ LPSECURITY_ATTRIBUTES lpThreadAttributes,
-    /* __in        */ BOOL bInheritHandles,
-    /* __in        */ DWORD dwCreationFlags,
     /* __in_opt    */ LPVOID lpEnvironment,
     /* __in_opt    */ LPCWSTR lpCurrentDirectory,
     /* __in        */ LPSTARTUPINFOW lpStartupInfo,
     /* __out       */ LPPROCESS_INFORMATION lpProcessInformation,
     /* __in        */ DWORD cHandlesToInherit,
-    /* __in_ecount(cHandlesToInherit) */ HANDLE* rgHandlesToInherit) {
-  BOOL fSuccess;
-  BOOL fInitialized = FALSE;
+    /* __in_ecount(cHandlesToInherit) */ HANDLE* handlesToInherit) {
+  if (wcsnlen_s(lpCommandLine, MAX_CMDLINE) == MAX_CMDLINE) {
+    std::wstring cmd_sample(lpCommandLine, 200);
+    cmd_sample.append(L"(...)");
+    std::wstringstream error_msg;
+    error_msg << L"command is longer than CreateProcessW's limit ("
+              << MAX_CMDLINE << L" characters)";
+    return bazel::windows::MakeErrorMessage(
+        WSTR(__FILE__), __LINE__, L"CreateProcessWithExplicitHandles",
+        cmd_sample.c_str(), error_msg.str().c_str());
+  }
+
+  if (cHandlesToInherit >= 0xFFFFFFFF / sizeof(HANDLE)) {
+    return bazel::windows::MakeErrorMessage(
+        WSTR(__FILE__), __LINE__, L"CreateProcessWithExplicitHandles",
+        lpCommandLine, L"too many handles to inherit");
+  }
+
+  if (lpStartupInfo->cb != sizeof(*lpStartupInfo)) {
+    return bazel::windows::MakeErrorMessage(
+        WSTR(__FILE__), __LINE__, L"CreateProcessWithExplicitHandles",
+        lpCommandLine, L"bad lpStartupInfo");
+  }
+
   SIZE_T size = 0;
-  LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList = NULL;
-
-  fSuccess = cHandlesToInherit < 0xFFFFFFFF / sizeof(HANDLE) &&
-             lpStartupInfo->cb == sizeof(*lpStartupInfo);
-  if (!fSuccess) {
-    SetLastError(ERROR_INVALID_PARAMETER);
-  }
-  if (fSuccess) {
-    fSuccess = InitializeProcThreadAttributeList(NULL, 1, 0, &size) ||
-               GetLastError() == ERROR_INSUFFICIENT_BUFFER;
-  }
-  if (fSuccess) {
-    lpAttributeList = reinterpret_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(
-        HeapAlloc(GetProcessHeap(), 0, size));
-    fSuccess = lpAttributeList != NULL;
-  }
-  if (fSuccess) {
-    fSuccess = InitializeProcThreadAttributeList(lpAttributeList, 1, 0, &size);
-  }
-  if (fSuccess) {
-    fInitialized = TRUE;
-    fSuccess = UpdateProcThreadAttribute(
-        lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
-        rgHandlesToInherit, cHandlesToInherit * sizeof(HANDLE), NULL, NULL);
-  }
-  if (fSuccess) {
-    STARTUPINFOEXW info;
-    ZeroMemory(&info, sizeof(info));
-    info.StartupInfo = *lpStartupInfo;
-    info.StartupInfo.cb = sizeof(info);
-    info.lpAttributeList = lpAttributeList;
-    fSuccess = CreateProcessW(
-        lpApplicationName, lpCommandLine, lpProcessAttributes,
-        lpThreadAttributes, bInheritHandles,
-        dwCreationFlags | EXTENDED_STARTUPINFO_PRESENT, lpEnvironment,
-        lpCurrentDirectory, &info.StartupInfo, lpProcessInformation);
-  }
-
-  if (fInitialized) {
-    DeleteProcThreadAttributeList(lpAttributeList);
+  if (!InitializeProcThreadAttributeList(NULL, 1, 0, &size) &&
+      GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
+    DWORD err_code = GetLastError();
+    return bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
+                                            L"CreateProcessWithExplicitHandles",
+                                            lpCommandLine, err_code);
   }
+
+  LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList =
+      reinterpret_cast<LPPROC_THREAD_ATTRIBUTE_LIST>(
+          HeapAlloc(GetProcessHeap(), 0, size));
+  if (lpAttributeList == NULL) {
+    DWORD err_code = GetLastError();
+    return bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
+                                            L"CreateProcessWithExplicitHandles",
+                                            lpCommandLine, err_code);
+  }
+
+  if (!InitializeProcThreadAttributeList(lpAttributeList, 1, 0, &size)) {
+    DWORD err_code = GetLastError();
+    return bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
+                                            L"CreateProcessWithExplicitHandles",
+                                            lpCommandLine, err_code);
+  }
+  if (!UpdateProcThreadAttribute(
+          lpAttributeList, 0, PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
+          handlesToInherit, cHandlesToInherit * sizeof(HANDLE), NULL, NULL)) {
+    DWORD err_code = GetLastError();
+    return bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
+                                            L"CreateProcessWithExplicitHandles",
+                                            lpCommandLine, err_code);
+  }
+
+  STARTUPINFOEXW info;
+  ZeroMemory(&info, sizeof(info));
+  info.StartupInfo = *lpStartupInfo;
+  info.StartupInfo.cb = sizeof(info);
+  info.lpAttributeList = lpAttributeList;
+  DWORD createproc_err = 0;
+  if (!CreateProcessW(
+          /* lpApplicationName */ NULL,
+          /* lpCommandLine */ lpCommandLine,
+          /* lpProcessAttributes */ NULL,
+          /* lpThreadAttributes */ NULL,
+          /* bInheritHandles */ TRUE,
+          /* dwCreationFlags */ CREATE_NO_WINDOW  // Don't create console window
+              | CREATE_NEW_PROCESS_GROUP  // So that Ctrl-Break isn't propagated
+              | CREATE_SUSPENDED  // So that it doesn't start a new job itself
+              | EXTENDED_STARTUPINFO_PRESENT,
+          /* lpEnvironment */ lpEnvironment,
+          /* lpCurrentDirectory */ lpCurrentDirectory,
+          /* lpStartupInfo */ &info.StartupInfo,
+          /* lpProcessInformation */ lpProcessInformation)) {
+    createproc_err = GetLastError();
+  }
+
+  DeleteProcThreadAttributeList(lpAttributeList);
   if (lpAttributeList) {
     HeapFree(GetProcessHeap(), 0, lpAttributeList);
   }
-  return fSuccess;
+  if (createproc_err) {
+    return bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
+                                            L"CreateProcessW", lpCommandLine,
+                                            createproc_err);
+  }
+  return L"";
 }
 
 extern "C" JNIEXPORT jlong JNICALL
@@ -388,26 +431,17 @@ Java_com_google_devtools_build_lib_windows_jni_WindowsProcesses_nativeCreateProc
   startup_info.dwFlags |= STARTF_USESTDHANDLES;
 
   HANDLE handlesToInherit[3] = {stdin_process, stdout_process, stderr_handle};
-  BOOL ok = CreateProcessWithExplicitHandles(
-      /* lpApplicationName */ NULL,
+  std::wstring err_msg(CreateProcessWithExplicitHandles(
       /* lpCommandLine */ mutable_commandline.get(),
-      /* lpProcessAttributes */ NULL,
-      /* lpThreadAttributes */ NULL,
-      /* bInheritHandles */ TRUE,
-      /* dwCreationFlags */ CREATE_NO_WINDOW  // Don't create a console window
-          | CREATE_NEW_PROCESS_GROUP  // So that Ctrl-Break is not propagated
-          | CREATE_SUSPENDED,  // So that it doesn't start a new job itself
       /* lpEnvironment */ env_map.ptr(),
       /* lpCurrentDirectory */ cwd.empty() ? nullptr : cwd.c_str(),
       /* lpStartupInfo */ &startup_info,
       /* lpProcessInformation */ &process_info,
       /* cHandlesToInherit */ (stderr_handle == stdout_process) ? 2 : 3,
-      /* rgHandlesToInherit */ handlesToInherit);
+      /* handlesToInherit */ handlesToInherit));
 
-  if (!ok) {
-    DWORD err_code = GetLastError();
-    result->error_ = bazel::windows::MakeErrorMessage(
-        WSTR(__FILE__), __LINE__, L"nativeCreateProcess", wpath, err_code);
+  if (!err_msg.empty()) {
+    result->error_ = err_msg;
     return PtrAsJlong(result);
   }