Windows, JNI: make it work with long paths

When spawning a new process with CreateProcessA,
convert argv0 to a 8dot3 style short path so we
can support longer paths than MAX_PATH. This is
the same approach we did in commit 44ecf9a.

See #2107
See #2181

--
PiperOrigin-RevId: 144613589
MOS_MIGRATED_REVID=144613589

src/main/java/com/google/devtools/build/lib/BUILD

@@ -78,15 +78,6 @@ java_library(
     ],
 )
 
-java_library(
-    name = "windows",
-    srcs = glob(["windows/*.java"]),
-    deps = [
-        ":shell",
-        "//third_party:guava",
-    ],
-)
-
 # Library of concurrency utilities.
 java_library(
     name = "concurrent",
@@ -134,6 +125,7 @@ java_library(
     srcs = glob([
         "profiler/*.java",
         "vfs/*.java",
+        "windows/*.java",
     ]),
     visibility = ["//visibility:public"],
     deps = [
@@ -142,8 +134,8 @@ java_library(
         ":concurrent",
         ":os_util",
         ":preconditions",
+        ":shell",
         ":unix",
-        ":windows",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
         "//third_party:jsr305",
@@ -272,7 +264,6 @@ java_library(
         ":shell",
         ":unix",
         ":vfs",
-        ":windows",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
         "//third_party:jsr305",
@@ -1012,7 +1003,6 @@ java_library(
         ":unix",
         ":util",
         ":vfs",
-        ":windows",
         "//src/main/java/com/google/devtools/build/docgen:docgen_javalib",
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",

src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java

@@ -36,9 +36,9 @@ private WindowsProcesses() {
    *
    * <p>Appropriately quoting arguments is the responsibility of the caller.
    *
-   * @param argv0 the binary to run; must be a Windows style path (with backslashes) but needs not
-   *     be quoted; may be something on the PATH (e.g. "cmd.exe") or an absolute path, in which case
-   *     it must be normalized
+   * @param argv0 the binary to run; must be unquoted; must be either an absolute, normalized
+   *     Windows path with a drive letter (e.g. "c:\foo\bar app.exe") or a single file name (e.g.
+   *     "foo app.exe")
    * @param argvRest the rest of the command line, i.e. argv[1:] (needs to be quoted Windows style)
    * @param commandLine the command line (needs to be quoted Windows style)
    * @param env the environment of the new process. null means inherit that of the Bazel server

src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java

@@ -18,6 +18,7 @@
 import com.google.devtools.build.lib.shell.Subprocess;
 import com.google.devtools.build.lib.shell.SubprocessBuilder;
 import com.google.devtools.build.lib.shell.SubprocessBuilder.StreamAction;
+import com.google.devtools.build.lib.vfs.PathFragment;
 import java.io.File;
 import java.io.IOException;
 import java.util.List;
@@ -39,7 +40,9 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
     WindowsJniLoader.loadJni();
 
     List<String> argv = builder.getArgv();
-    String argv0 = WindowsProcesses.quoteCommandLine(argv.subList(0, 1));
+
+    // DO NOT quote argv0, nativeCreateProcess will do it for us.
+    String argv0 = processArgv0(argv.get(0));
     String argvRest =
         argv.size() > 1 ? WindowsProcesses.quoteCommandLine(argv.subList(1, argv.size())) : "";
     byte[] env = builder.getEnv() == null ? null : convertEnvToNative(builder.getEnv());
@@ -64,6 +67,21 @@ public Subprocess create(SubprocessBuilder builder) throws IOException {
         builder.getTimeoutMillis());
   }
 
+  public String processArgv0(String argv0) {
+    // Normalize the path and make it Windows-style.
+    // If argv0 is longer than MAX_PATH (260 chars), createNativeProcess calls GetShortPathNameW to
+    // obtain a 8dot3 name for it (thereby support long paths in CreateProcessA), but then argv0
+    // must be prefixed with "\\?\" for GetShortPathNameW to work, so it also must be an absolute,
+    // normalized, Windows-style path.
+    // Therefore if it's absolute, then normalize it also.
+    // If it's not absolute, then it cannot be longer than MAX_PATH, since MAX_PATH also limits the
+    // length of file names.
+    PathFragment argv0fragment = new PathFragment(argv0);
+    return (argv0fragment.isAbsolute())
+        ? argv0fragment.normalize().getPathString().replace('/', '\\')
+        : argv0;
+  }
+
   private String getRedirectPath(StreamAction action, File file) {
     switch (action) {
       case DISCARD:

src/main/native/windows_processes.cc

@@ -15,7 +15,9 @@
 #define WINVER 0x0601
 #define _WIN32_WINNT 0x0601
 
+#include <ctype.h>
 #include <jni.h>
+#include <stdlib.h>
 #include <string.h>
 #include <windows.h>
 
@@ -153,14 +155,127 @@ static std::wstring AddUncPrefixMaybe(const std::wstring& path) {
 
 static jlong PtrAsJlong(void* p) { return reinterpret_cast<jlong>(p); }
 
+static void QuotePath(const std::string& path, std::string* result) {
+  *result = std::string("\"") + path + "\"";
+}
+
+// Computes a path suitable as the executable part in CreateProcessA's cmdline.
+//
+// The null-terminated executable path for CreateProcessA has to fit into
+// MAX_PATH, therefore the limit for the executable's path is MAX_PATH - 1
+// (not including null terminator).
+//
+// `path` must be either an absolute, normalized, Windows-style path with drive
+// letter (e.g. "c:\foo\bar.exe", but no "\foo\bar.exe"), or must be just a file
+// name (e.g. "cmd.exe") that's shorter than MAX_PATH (without null-terminator).
+// In both cases, `path` must be unquoted.
+//
+// If this function succeeds, it returns an empty string (indicating no error),
+// and sets `result` to the resulting path, which is always quoted, and is
+// always at most MAX_PATH + 1 long (MAX_PATH - 1 without null terminator, plus
+// two quotes). If there's any error, this function returns the error message.
+//
+// If `path` is at most MAX_PATH - 1 long (not including null terminator), the
+// result will be that (plus quotes).
+// Otherwise this method attempts to compute an 8dot3 style short name for
+// `path`, and if that succeeds and the result is at most MAX_PATH - 1 long (not
+// including null terminator), then that will be the result (plus quotes).
+// Otherwise this function fails and returns an error message.
+static std::string AsExecutableForCreateProcess(JNIEnv* env, jstring path,
+                                                std::string* result) {
+  std::string spath = GetJavaUTFString(env, path);
+  if (spath.empty()) {
+    return std::string("argv[0] should not be empty");
+  }
+  if (spath[0] == '"') {
+    return std::string("argv[0] should not be quoted");
+  }
+  if (spath[0] == '\\' ||  // absolute, but without drive letter
+      spath.find("/") != std::string::npos ||       // has "/"
+      spath.find("\\.\\") != std::string::npos ||   // not normalized
+      spath.find("\\..\\") != std::string::npos ||  // not normalized
+      // at least MAX_PATH long, but just a file name
+      (spath.size() >= MAX_PATH && spath.find_first_of('\\') == string::npos) ||
+      // not just a file name, but also not absolute
+      (spath.find_first_of('\\') != string::npos &&
+       !(isalpha(spath[0]) && spath[1] == ':' && spath[2] == '\\'))) {
+    return std::string("argv[0]='" + spath +
+                       "'; should have been either an absolute, "
+                       "normalized, Windows-style path with drive letter (e.g. "
+                       "'c:\\foo\\bar.exe'), or just a file name (e.g. "
+                       "'cmd.exe') shorter than MAX_PATH.");
+  }
+  // At this point we know the path is either just a file name (shorter than
+  // MAX_PATH), or an absolute, normalized, Windows-style path (of any length).
+
+  // Fast-track: the path is already short.
+  if (spath.size() < MAX_PATH) {
+    // Quote the path in case it's something like "c:\foo\app name.exe".
+    // Do this unconditionally, there's no harm in quoting. Quotes are not
+    // allowed inside paths so we don't need to escape quotes.
+    QuotePath(spath, result);
+    return "";
+  }
+  // At this point we know that the path is at least MAX_PATH long and that it's
+  // absolute, normalized, and Windows-style.
+
+  // Retrieve string as UTF-16 path, add "\\?\" prefix.
+  std::wstring wlong = std::wstring(L"\\\\?\\") + GetJavaWstring(env, path);
+
+  // Experience shows that:
+  // - GetShortPathNameW's result has a "\\?\" prefix if and only if the input
+  //   did too (though this behavior is not documented on MSDN)
+  // - CreateProcess{A,W} only accept an executable of MAX_PATH - 1 length
+  // Therefore for our purposes the acceptable shortened length is
+  // MAX_PATH + 4 (null-terminated). That is, MAX_PATH - 1 for the shortened
+  // path, plus a potential "\\?\" prefix that's only there if `wlong` also had
+  // it and which we'll omit from `result`, plus a null terminator.
+  static const size_t kMaxShortPath = MAX_PATH + 4;
+
+  WCHAR wshort[kMaxShortPath];
+  DWORD wshort_size = ::GetShortPathNameW(wlong.c_str(), NULL, 0);
+  if (wshort_size == 0) {
+    return windows_util::GetLastErrorString(
+        std::string("GetShortPathName failed (path=") + spath + ")");
+  }
+
+  if (wshort_size >= kMaxShortPath) {
+    return windows_util::GetLastErrorString(
+        std::string(
+            "GetShortPathName would not shorten the path enough (path=") +
+        spath + ")");
+  }
+
+  // Convert the result to UTF-8.
+  char mbs_short[MAX_PATH];
+  size_t mbs_size = wcstombs(
+      mbs_short,
+      wshort + 4,  // we know it has a "\\?\" prefix, because `wlong` also did
+      MAX_PATH);
+  if (mbs_size < 0 || mbs_size >= MAX_PATH) {
+    return std::string("wcstombs failed (path=") + spath + ")";
+  }
+  mbs_short[mbs_size - 1] = 0;
+
+  QuotePath(mbs_short, result);
+  return "";
+}
+
 extern "C" JNIEXPORT jlong JNICALL
 Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeCreateProcess(
     JNIEnv* env, jclass clazz, jstring java_argv0, jstring java_argv_rest,
     jbyteArray java_env, jstring java_cwd, jstring java_stdout_redirect,
     jstring java_stderr_redirect) {
-  // TODO(laszlocsomor): compute the 8dot3 path for java_argv0.
-  std::string commandline = GetJavaUTFString(env, java_argv0) + " " +
-                            GetJavaUTFString(env, java_argv_rest);
+  NativeProcess* result = new NativeProcess();
+
+  std::string argv0;
+  std::string error_msg(AsExecutableForCreateProcess(env, java_argv0, &argv0));
+  if (!error_msg.empty()) {
+    result->error_ = error_msg;
+    return PtrAsJlong(result);
+  }
+
+  std::string commandline = argv0 + " " + GetJavaUTFString(env, java_argv_rest);
   std::wstring stdout_redirect =
       AddUncPrefixMaybe(GetJavaWstring(env, java_stdout_redirect));
   std::wstring stderr_redirect =
@@ -171,8 +286,6 @@ Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeCreateProcess(
   strncpy(mutable_commandline.get(), commandline.c_str(),
           commandline.size() + 1);
 
-  NativeProcess* result = new NativeProcess();
-
   SECURITY_ATTRIBUTES sa = {0};
   sa.nLength = sizeof(SECURITY_ATTRIBUTES);
   sa.bInheritHandle = TRUE;

src/test/java/com/google/devtools/build/lib/BUILD

@@ -194,7 +194,6 @@ java_test(
         "//src/main/java/com/google/devtools/build/lib:inmemoryfs",
         "//src/main/java/com/google/devtools/build/lib:util",
         "//src/main/java/com/google/devtools/build/lib:vfs",
-        "//src/main/java/com/google/devtools/build/lib:windows",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:guava",
         "//third_party:guava-testlib",
@@ -210,7 +209,6 @@ java_library(
     ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib:vfs",
-        "//src/main/java/com/google/devtools/build/lib:windows",
         "//third_party:guava",
         "//third_party:guava-testlib",
         "//third_party:junit4",
@@ -232,7 +230,6 @@ java_test(
         ":windows_testutil",
         "//src/main/java/com/google/devtools/build/lib:os_util",
         "//src/main/java/com/google/devtools/build/lib:vfs",
-        "//src/main/java/com/google/devtools/build/lib:windows",
         "//third_party:guava",
         "//third_party:truth",
     ],

src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java

@@ -47,7 +47,7 @@ public class WindowsProcessesTest {
   public void loadJni() throws Exception {
     mockSubprocess = WindowsTestUtil.getRunfile(
         "io_bazel/src/test/java/com/google/devtools/build/lib/MockSubprocess_deploy.jar");
-    mockBinary = System.getProperty("java.home") + "/bin/java";
+    mockBinary = System.getProperty("java.home") + "\\bin\\java.exe";
 
     WindowsJniLoader.loadJni();