Windows, JNI: arg for argv0 in nativeCreateProcess Add a separate argument to nativeCreateProcess for argv[0] specifically, and another for the rest of the args. In a subsequent change I'll add code to compute the 8dot3 style short name of the argv[0] so we can use longer paths for executables in CreateProcessA than we normally could. This is the same approach as used in commit 44ecf9a0c7c25496a43f59f1c8f20df9527e12cb See https://github.com/bazelbuild/bazel/issues/2107 See https://github.com/bazelbuild/bazel/issues/2181 -- PiperOrigin-RevId: 144311562 MOS_MIGRATED_REVID=144311562
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 67d6362..6c6b3a0 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,6 +36,10 @@ * * <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 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 * @param cwd the working directory of the new process. if null, the same as that of the current @@ -46,8 +50,8 @@ * work. * @return the opaque identifier of the created process */ - static native long nativeCreateProcess(String commandLine, byte[] env, - String cwd, String stdoutFile, String stderrFile); + static native long nativeCreateProcess( + String argv0, String argvRest, byte[] env, String cwd, String stdoutFile, String stderrFile); /** * Writes data from the given array to the stdin of the specified process.
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 965a186..9f6f0eb 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,9 +18,9 @@ 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 java.io.File; import java.io.IOException; +import java.util.List; import java.util.Map; import java.util.TreeMap; @@ -38,22 +38,30 @@ public Subprocess create(SubprocessBuilder builder) throws IOException { WindowsJniLoader.loadJni(); - String commandLine = WindowsProcesses.quoteCommandLine(builder.getArgv()); + List<String> argv = builder.getArgv(); + String argv0 = WindowsProcesses.quoteCommandLine(argv.subList(0, 1)); + String argvRest = + argv.size() > 1 ? WindowsProcesses.quoteCommandLine(argv.subList(1, argv.size())) : ""; byte[] env = builder.getEnv() == null ? null : convertEnvToNative(builder.getEnv()); String stdoutPath = getRedirectPath(builder.getStdout(), builder.getStdoutFile()); String stderrPath = getRedirectPath(builder.getStderr(), builder.getStderrFile()); - long nativeProcess = WindowsProcesses.nativeCreateProcess( - commandLine, env, builder.getWorkingDirectory().getPath(), stdoutPath, stderrPath); + long nativeProcess = + WindowsProcesses.nativeCreateProcess( + argv0, argvRest, env, builder.getWorkingDirectory().getPath(), stdoutPath, stderrPath); String error = WindowsProcesses.nativeProcessGetLastError(nativeProcess); if (!error.isEmpty()) { WindowsProcesses.nativeDeleteProcess(nativeProcess); throw new IOException(error); } - return new WindowsSubprocess(nativeProcess, commandLine, stdoutPath != null, - stderrPath != null, builder.getTimeoutMillis()); + return new WindowsSubprocess( + nativeProcess, + argv0 + " " + argvRest, + stdoutPath != null, + stderrPath != null, + builder.getTimeoutMillis()); } private String getRedirectPath(StreamAction action, File file) {
diff --git a/src/main/native/windows_processes.cc b/src/main/native/windows_processes.cc index 4935543..20bdd5c 100644 --- a/src/main/native/windows_processes.cc +++ b/src/main/native/windows_processes.cc
@@ -20,6 +20,7 @@ #include <windows.h> #include <atomic> +#include <memory> #include <string> #include "src/main/native/windows_util.h" @@ -80,34 +81,34 @@ version_info.dwMajorVersion == 6 && version_info.dwMinorVersion >= 2; } +static std::string GetJavaUTFString(JNIEnv* env, jstring str) { + std::string result; + if (str != nullptr) { + const char* jstr = env->GetStringUTFChars(str, nullptr); + result.assign(jstr); + env->ReleaseStringUTFChars(str, jstr); + } + return result; +} + extern "C" JNIEXPORT jlong JNICALL Java_com_google_devtools_build_lib_windows_WindowsProcesses_nativeCreateProcess( - JNIEnv *env, jclass clazz, jstring java_commandline, jbyteArray java_env, - jstring java_cwd, jstring java_stdout_redirect, - jstring java_stderr_redirect) { - const char* commandline = env->GetStringUTFChars(java_commandline, NULL); - const char* stdout_redirect = NULL; - const char* stderr_redirect = NULL; - const char* cwd = NULL; - - if (java_stdout_redirect != NULL) { - stdout_redirect = env->GetStringUTFChars(java_stdout_redirect, NULL); - } - - if (java_stderr_redirect != NULL) { - stderr_redirect = env->GetStringUTFChars(java_stderr_redirect, NULL); - } - - if (java_cwd != NULL) { - cwd = env->GetStringUTFChars(java_cwd, NULL); - } + 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); + std::string stdout_redirect = GetJavaUTFString(env, java_stdout_redirect); + std::string stderr_redirect = GetJavaUTFString(env, java_stderr_redirect); + std::string cwd = GetJavaUTFString(env, java_cwd); jsize env_size = -1; jbyte* env_bytes = NULL; - - char* mutable_commandline = new char[strlen(commandline) + 1]; - strncpy(mutable_commandline, commandline, strlen(commandline) + 1); + std::unique_ptr<char[]> mutable_commandline(new char[commandline.size() + 1]); + strncpy(mutable_commandline.get(), commandline.c_str(), + commandline.size() + 1); NativeProcess* result = new NativeProcess(); @@ -142,17 +143,17 @@ goto cleanup; } - if (stdout_redirect != NULL) { + if (!stdout_redirect.empty()) { result->stdout_.close(); stdout_process = CreateFileA( - stdout_redirect, - FILE_APPEND_DATA, - 0, - &sa, - OPEN_ALWAYS, - FILE_ATTRIBUTE_NORMAL, - NULL); + /* lpFileName */ stdout_redirect.c_str(), + /* dwDesiredAccess */ FILE_APPEND_DATA, + /* dwShareMode */ 0, + /* lpSecurityAttributes */ &sa, + /* dwCreationDisposition */ OPEN_ALWAYS, + /* dwFlagsAndAttributes */ FILE_ATTRIBUTE_NORMAL, + /* hTemplateFile */ NULL); if (stdout_process == INVALID_HANDLE_VALUE) { result->error_ = windows_util::GetLastErrorString("CreateFile(stdout)"); @@ -165,19 +166,19 @@ } } - if (stderr_redirect != NULL) { + if (!stderr_redirect.empty()) { result->stderr_.close(); - if (!strcmp(stdout_redirect, stderr_redirect)) { + if (stdout_redirect == stderr_redirect) { stderr_process = stdout_process; } else { stderr_process = CreateFileA( - stderr_redirect, - FILE_APPEND_DATA, - 0, - &sa, - OPEN_ALWAYS, - FILE_ATTRIBUTE_NORMAL, - NULL); + /* lpFileName */ stderr_redirect.c_str(), + /* dwDesiredAccess */ FILE_APPEND_DATA, + /* dwShareMode */ 0, + /* lpSecurityAttributes */ &sa, + /* dwCreationDisposition */ OPEN_ALWAYS, + /* dwFlagsAndAttributes */ FILE_ATTRIBUTE_NORMAL, + /* hTemplateFile */ NULL); if (stderr_process == INVALID_HANDLE_VALUE) { result->error_ = windows_util::GetLastErrorString("CreateFile(stderr)"); @@ -191,7 +192,6 @@ } } - // MDSN says that the default for job objects is that breakaway is not // allowed. Thus, we don't need to do any more setup here. HANDLE job = CreateJobObject(NULL, NULL); @@ -220,18 +220,18 @@ startup_info.dwFlags |= STARTF_USESTDHANDLES; BOOL ok = CreateProcessA( - NULL, - mutable_commandline, - NULL, - NULL, - TRUE, - CREATE_NO_WINDOW // Don't create a console window - | CREATE_NEW_PROCESS_GROUP // So that Ctrl-Break is not propagated + /* lpApplicationName */ NULL, + /* 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 - env_bytes, - cwd, - &startup_info, - &process_info); + /* lpEnvironment */ env_bytes, + /* lpCurrentDirectory */ cwd.empty() ? nullptr : cwd.c_str(), + /* lpStartupInfo */ &startup_info, + /* lpProcessInformation */ &process_info); if (!ok) { result->error_ = windows_util::GetLastErrorString("CreateProcess()"); @@ -289,23 +289,9 @@ CloseHandle(thread); } - delete[] mutable_commandline; if (env_bytes != NULL) { env->ReleaseByteArrayElements(java_env, env_bytes, 0); } - env->ReleaseStringUTFChars(java_commandline, commandline); - - if (stdout_redirect != NULL) { - env->ReleaseStringUTFChars(java_stdout_redirect, stdout_redirect); - } - - if (stderr_redirect != NULL) { - env->ReleaseStringUTFChars(java_stderr_redirect, stderr_redirect); - } - - if (cwd != NULL) { - env->ReleaseStringUTFChars(java_cwd, cwd); - } return reinterpret_cast<jlong>(result); }
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 61c935d..1ae3cde 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
@@ -40,14 +40,14 @@ public class WindowsProcessesTest { private static final Charset UTF8 = Charset.forName("UTF-8"); private String mockSubprocess; - private String javaHome; + private String mockBinary; private long process; @Before public void loadJni() throws Exception { mockSubprocess = WindowsTestUtil.getRunfile( "io_bazel/src/test/java/com/google/devtools/build/lib/MockSubprocess_deploy.jar"); - javaHome = System.getProperty("java.home"); + mockBinary = System.getProperty("java.home") + "/bin/java"; WindowsJniLoader.loadJni(); @@ -65,7 +65,6 @@ private String mockArgs(String... args) { List<String> argv = new ArrayList<>(); - argv.add(javaHome + "/bin/java"); argv.add("-jar"); argv.add(mockSubprocess); for (String arg : args) { @@ -85,7 +84,9 @@ @Test public void testSmoke() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs("Ia5", "Oa"), null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("Ia5", "Oa"), null, null, null, null); assertNoProcessError(); byte[] input = "HELLO".getBytes(UTF8); @@ -105,8 +106,9 @@ args.add("Oa"); } - process = WindowsProcesses.nativeCreateProcess(mockArgs(args.toArray(new String[] {})), null, - null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs(args.toArray(new String[] {})), null, null, null, null); for (int i = 0; i < 100; i++) { byte[] input = String.format("%03d", i).getBytes(UTF8); assertThat(input.length).isEqualTo(3); @@ -129,7 +131,8 @@ @Test public void testExitCode() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs("X42"), null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs("X42"), null, null, null, null); assertThat(WindowsProcesses.nativeWaitFor(process, -1)).isEqualTo(0); assertThat(WindowsProcesses.nativeGetExitCode(process)).isEqualTo(42); assertNoProcessError(); @@ -137,7 +140,9 @@ @Test public void testPartialRead() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs("O-HELLO"), null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("O-HELLO"), null, null, null, null); byte[] one = new byte[2]; byte[] two = new byte[3]; @@ -152,7 +157,8 @@ @Test public void testArrayOutOfBounds() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs("O-oob"), null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs("O-oob"), null, null, null, null); byte[] buf = new byte[3]; assertThat(readStdout(buf, -1, 3)).isEqualTo(-1); assertThat(readStdout(buf, 0, 5)).isEqualTo(-1); @@ -181,7 +187,9 @@ @Test public void testOffsetedOps() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs("Ia3", "Oa"), null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("Ia3", "Oa"), null, null, null, null); byte[] input = "01234".getBytes(UTF8); byte[] output = "abcde".getBytes(UTF8); @@ -196,9 +204,15 @@ @Test public void testParallelStdoutAndStderr() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs( - "O-out1", "E-err1", "O-out2", "E-err2", "E-err3", "O-out3", "E-err4", "O-out4"), - null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, + mockArgs( + "O-out1", "E-err1", "O-out2", "E-err2", "E-err3", "O-out3", "E-err4", "O-out4"), + null, + null, + null, + null); byte[] buf = new byte[4]; assertThat(readStdout(buf, 0, 4)).isEqualTo(4); @@ -224,8 +238,9 @@ @Test public void testExecutableNotFound() throws Exception { - process = WindowsProcesses.nativeCreateProcess("ThisExecutableDoesNotExist", - null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + "ThisExecutableDoesNotExist", "TheseArgsDontMatter", null, null, null, null); assertThat(WindowsProcesses.nativeProcessGetLastError(process)) .contains("The system cannot find the file specified."); byte[] buf = new byte[1]; @@ -234,7 +249,8 @@ @Test public void testReadingAndWritingAfterTermination() throws Exception { - process = WindowsProcesses.nativeCreateProcess("X42", null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs("X42"), null, null, null, null); byte[] buf = new byte[1]; assertThat(readStdout(buf, 0, 1)).isEqualTo(0); assertThat(readStderr(buf, 0, 1)).isEqualTo(0); @@ -244,8 +260,9 @@ @Test public void testNewEnvironmentVariables() throws Exception { byte[] data = "ONE=one\0TWO=twotwo\0\0".getBytes(UTF8); - process = WindowsProcesses.nativeCreateProcess( - mockArgs("O$ONE", "O$TWO"), data, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("O$ONE", "O$TWO"), data, null, null, null); assertNoProcessError(); byte[] buf = new byte[3]; assertThat(readStdout(buf, 0, 3)).isEqualTo(3); @@ -258,35 +275,35 @@ @Test public void testNoZeroInEnvBuffer() throws Exception { byte[] data = "clown".getBytes(UTF8); - process = WindowsProcesses.nativeCreateProcess(mockArgs(), data, null, null, null); + process = WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs(), data, null, null, null); assertThat(WindowsProcesses.nativeProcessGetLastError(process)).isNotEmpty(); } @Test public void testMissingFinalDoubleZeroInEnvBuffer() throws Exception { byte[] data = "FOO=bar\0".getBytes(UTF8); - process = WindowsProcesses.nativeCreateProcess(mockArgs(), data, null, null, null); + process = WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs(), data, null, null, null); assertThat(WindowsProcesses.nativeProcessGetLastError(process)).isNotEmpty(); } @Test public void testOneByteEnvBuffer() throws Exception { byte[] data = "a".getBytes(UTF8); - process = WindowsProcesses.nativeCreateProcess(mockArgs(), data, null, null, null); + process = WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs(), data, null, null, null); assertThat(WindowsProcesses.nativeProcessGetLastError(process)).isNotEmpty(); } @Test public void testOneZeroEnvBuffer() throws Exception { byte[] data = "\0".getBytes(UTF8); - process = WindowsProcesses.nativeCreateProcess(mockArgs(), data, null, null, null); + process = WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs(), data, null, null, null); assertThat(WindowsProcesses.nativeProcessGetLastError(process)).isNotEmpty(); } @Test public void testTwoZerosInEnvBuffer() throws Exception { byte[] data = "\0\0".getBytes(UTF8); - process = WindowsProcesses.nativeCreateProcess(mockArgs(), data, null, null, null); + process = WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs(), data, null, null, null); assertThat(WindowsProcesses.nativeProcessGetLastError(process)).isEmpty(); } @@ -295,8 +312,9 @@ String stdoutFile = System.getenv("TEST_TMPDIR") + "\\stdout_redirect"; String stderrFile = System.getenv("TEST_TMPDIR") + "\\stderr_redirect"; - process = WindowsProcesses.nativeCreateProcess(mockArgs("O-one", "E-two"), - null, null, stdoutFile, stderrFile); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("O-one", "E-two"), null, null, stdoutFile, stderrFile); assertThat(process).isGreaterThan(0L); assertNoProcessError(); assertThat(WindowsProcesses.nativeWaitFor(process, -1)).isEqualTo(0); @@ -312,8 +330,9 @@ public void testRedirectToSameFile() throws Exception { String file = System.getenv("TEST_TMPDIR") + "\\captured_"; - process = WindowsProcesses.nativeCreateProcess(mockArgs("O-one", "E-two"), - null, null, file, file); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("O-one", "E-two"), null, null, file, file); assertThat(process).isGreaterThan(0L); assertNoProcessError(); assertThat(WindowsProcesses.nativeWaitFor(process, -1)).isEqualTo(0); @@ -328,8 +347,9 @@ String stdoutFile = System.getenv("TEST_TMPDIR") + "\\captured_stdout"; String stderrFile = System.getenv("TEST_TMPDIR") + "\\captured_stderr"; - process = WindowsProcesses.nativeCreateProcess(mockArgs("O-one", "E-two"), null, null, - stdoutFile, stderrFile); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("O-one", "E-two"), null, null, stdoutFile, stderrFile); assertNoProcessError(); byte[] buf = new byte[1]; assertThat(readStdout(buf, 0, 1)).isEqualTo(0); @@ -346,8 +366,9 @@ Files.write(stdout, "out1".getBytes(UTF8)); Files.write(stderr, "err1".getBytes(UTF8)); - process = WindowsProcesses.nativeCreateProcess(mockArgs("O-out2", "E-err2"), null, - null, stdoutFile, stderrFile); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("O-out2", "E-err2"), null, null, stdoutFile, stderrFile); assertNoProcessError(); WindowsProcesses.nativeWaitFor(process, -1); WindowsProcesses.nativeGetExitCode(process); @@ -363,7 +384,8 @@ String dir1 = System.getenv("TEST_TMPDIR") + "/dir1"; new File(dir1).mkdir(); - process = WindowsProcesses.nativeCreateProcess(mockArgs("O."), null, dir1, null, null); + process = + WindowsProcesses.nativeCreateProcess(mockBinary, mockArgs("O."), null, dir1, null, null); assertNoProcessError(); byte[] buf = new byte[1024]; // Windows MAX_PATH is 256, but whatever int len = readStdout(buf, 0, 1024); @@ -373,7 +395,9 @@ @Test public void testTimeout() throws Exception { - process = WindowsProcesses.nativeCreateProcess(mockArgs("W5", "X0"), null, null, null, null); + process = + WindowsProcesses.nativeCreateProcess( + mockBinary, mockArgs("W5", "X0"), null, null, null, null); assertThat(WindowsProcesses.nativeWaitFor(process, 1000)).isEqualTo(1); } }