From 241badf321c4cbf91728b43e5754f68394e0af4a Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 21 May 2024 08:03:43 -0700 Subject: [PATCH] Switch CredentialHelper to WindowsSubprocessFactory on Windows and increase the stdout/stderr pipe size to 64KB. Fixes #21287 by preventing a deadlock/timeout when a credential helper response is too big, while avoiding more complex solutions for now. 64KB ought to be enough for anybody (TM). PiperOrigin-RevId: 635808494 Change-Id: I29c8d79ae1c92740e7400ba4074c835dcea7de14 --- .../credentialhelper/CredentialHelper.java | 5 +---- src/main/native/windows/processes-jni.cc | 9 +++++--- .../CredentialHelperTest.java | 22 ++++++++++++++++++- .../test_credential_helper.py | 8 ++++++- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java index 42d42b13af2ae2..e080874e897f0d 100644 --- a/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java +++ b/src/main/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelper.java @@ -23,7 +23,6 @@ import com.google.common.io.CharStreams; import com.google.devtools.build.lib.profiler.Profiler; import com.google.devtools.build.lib.profiler.SilentCloseable; -import com.google.devtools.build.lib.shell.JavaSubprocessFactory; import com.google.devtools.build.lib.shell.Subprocess; import com.google.devtools.build.lib.shell.SubprocessBuilder; import com.google.devtools.build.lib.vfs.Path; @@ -163,9 +162,7 @@ private Subprocess spawnSubprocess(CredentialHelperEnvironment environment, Stri Preconditions.checkNotNull(environment); Preconditions.checkNotNull(args); - // Force using JavaSubprocessFactory on Windows, because for some reasons, - // WindowsSubprocessFactory cannot redirect stdin to subprocess. - return new SubprocessBuilder(JavaSubprocessFactory.INSTANCE) + return new SubprocessBuilder() .setArgv(ImmutableList.builder().add(path.getPathString()).add(args).build()) .setWorkingDirectory( environment.getWorkspacePath() != null diff --git a/src/main/native/windows/processes-jni.cc b/src/main/native/windows/processes-jni.cc index df410736e6cbe6..051fdc43b067aa 100644 --- a/src/main/native/windows/processes-jni.cc +++ b/src/main/native/windows/processes-jni.cc @@ -33,6 +33,9 @@ #include "src/main/native/windows/process.h" #include "src/main/native/windows/util.h" +// Pipe buffer size, to match the Linux/MacOS default. +#define PIPE_SIZE 65536 + template static std::wstring ToString(const T& e) { std::wstringstream s; @@ -242,7 +245,7 @@ class NativeProcess { // Set up childs stdin pipe. { HANDLE pipe_read_h, pipe_write_h; - if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, 0)) { + if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, PIPE_SIZE)) { DWORD err_code = GetLastError(); error_ = bazel::windows::MakeErrorMessage( WSTR(__FILE__), __LINE__, L"nativeCreateProcess", wpath, err_code); @@ -290,7 +293,7 @@ class NativeProcess { } } else { HANDLE pipe_read_h, pipe_write_h; - if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, 0)) { + if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, PIPE_SIZE)) { DWORD err_code = GetLastError(); error_ = bazel::windows::MakeErrorMessage( WSTR(__FILE__), __LINE__, L"nativeCreateProcess", wpath, err_code); @@ -351,7 +354,7 @@ class NativeProcess { } } else { HANDLE pipe_read_h, pipe_write_h; - if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, 0)) { + if (!CreatePipe(&pipe_read_h, &pipe_write_h, &sa, PIPE_SIZE)) { DWORD err_code = GetLastError(); error_ = bazel::windows::MakeErrorMessage( WSTR(__FILE__), __LINE__, L"nativeCreateProcess", wpath, err_code); diff --git a/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperTest.java b/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperTest.java index 6ba4e1a02587b5..c7707b529395c8 100644 --- a/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperTest.java +++ b/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/CredentialHelperTest.java @@ -103,6 +103,7 @@ public void unknownUri() { assertThrows( CredentialHelperException.class, () -> getCredentialsFromHelper("https://unknown.example.com")); + assertThat(e).hasMessageThat().contains("Failed to get credentials"); assertThat(e).hasMessageThat().contains("Unknown uri 'https://unknown.example.com'"); } @@ -112,6 +113,7 @@ public void credentialHelperOutputsNothing() throws Exception { assertThrows( CredentialHelperException.class, () -> getCredentialsFromHelper("https://printnothing.example.com")); + assertThat(e).hasMessageThat().contains("Failed to get credentials"); assertThat(e).hasMessageThat().contains("exited without output"); } @@ -145,6 +147,7 @@ public void helperTimeout() throws Exception { assertThrows( CredentialHelperException.class, () -> getCredentialsFromHelper("https://timeout.example.com")); + assertThat(e).hasMessageThat().contains("Failed to get credentials"); assertThat(e).hasMessageThat().contains("process timed out"); } @@ -158,6 +161,23 @@ public void nonExistentHelper() throws Exception { OS.getCurrent() == OS.WINDOWS ? "C:/no/such/file" : "/no/such/file", "https://timeout.example.com", ImmutableMap.of())); - assertThat(e).hasMessageThat().contains("Cannot run program"); + assertThat(e).hasMessageThat().contains("Failed to get credentials"); + assertThat(e) + .hasMessageThat() + .contains( + OS.getCurrent().equals(OS.WINDOWS) + ? "cannot find the file specified" + : "Cannot run program"); + } + + @Test + public void hugePayload() throws Exception { + // Bazel reads the credential helper stdout/stderr from a pipe, and doesn't start reading + // until the process terminates. Therefore, a response larger than the pipe buffer causes + // a deadlock and timeout. This verifies that the pipe is sufficiently large. + // See https://github.com/bazelbuild/bazel/issues/21287. + GetCredentialsResponse response = getCredentialsFromHelper("https://hugepayload.example.com"); + assertThat(response.getHeaders()) + .containsExactly("huge", ImmutableList.of("x".repeat(63 * 1024))); } } diff --git a/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/test_credential_helper.py b/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/test_credential_helper.py index c21fd7b22524e6..101cab0d5db637 100644 --- a/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/test_credential_helper.py +++ b/src/test/java/com/google/devtools/build/lib/authandtls/credentialhelper/test_credential_helper.py @@ -60,6 +60,12 @@ def main(argv): } elif request["uri"] == "https://printnothing.example.com": return 0 + elif request["uri"] == "https://hugepayload.example.com": + response = { + "headers": { + "huge": [63 * 1024 * "x"], + }, + } elif request["uri"] == "https://cwd.example.com": response = { "headers": { @@ -84,7 +90,7 @@ def main(argv): else: eprint("Unknown uri '{}'".format(request["uri"])) return 1 - json.dump(response, sys.stdout) + sys.stdout.write(json.dumps(response)) return 0