From e68c6b5128119362c5e952638d4db0b269d2e4af Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Mon, 16 Jan 2017 10:03:29 +0000 Subject: [PATCH] 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 44ecf9a0c7c25496a43f59f1c8f20df9527e12cb. See https://github.com/bazelbuild/bazel/issues/2107 See https://github.com/bazelbuild/bazel/issues/2181 -- PiperOrigin-RevId: 144613589 MOS_MIGRATED_REVID=144613589 --- .../java/com/google/devtools/build/lib/BUILD | 14 +- .../build/lib/windows/WindowsProcesses.java | 6 +- .../lib/windows/WindowsSubprocessFactory.java | 20 ++- src/main/native/windows_processes.cc | 123 +++++++++++++++++- .../java/com/google/devtools/build/lib/BUILD | 3 - .../lib/windows/WindowsProcessesTest.java | 2 +- 6 files changed, 143 insertions(+), 25 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 967ce84b7970b9..9f9708107d4c66 100644 --- a/src/main/java/com/google/devtools/build/lib/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java index 6c6b3a07d56428..9aac2a7719bf5b 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsProcesses.java @@ -36,9 +36,9 @@ private WindowsProcesses() { * *

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 diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java index 9f6f0eb565c441..ae8b495cfda285 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java +++ b/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 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: diff --git a/src/main/native/windows_processes.cc b/src/main/native/windows_processes.cc index 0eb7e5f4187c3d..503b4b75b1bf42 100644 --- a/src/main/native/windows_processes.cc +++ b/src/main/native/windows_processes.cc @@ -15,7 +15,9 @@ #define WINVER 0x0601 #define _WIN32_WINNT 0x0601 +#include #include +#include #include #include @@ -153,14 +155,127 @@ static std::wstring AddUncPrefixMaybe(const std::wstring& path) { static jlong PtrAsJlong(void* p) { return reinterpret_cast(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; diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD index ff7b4ebad3d045..bc018d72999c61 100644 --- a/src/test/java/com/google/devtools/build/lib/BUILD +++ b/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", ], diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java index 1ae3cded74472a..b19ea488e7c2a4 100644 --- a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java +++ b/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();