Switch CredentialHelper to WindowsSubprocessFactory on Windows and in…

…crease 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

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.<String>builder().add(path.getPathString()).add(args).build())
         .setWorkingDirectory(
             environment.getWorkspacePath() != null

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 <typename T>
 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);

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)));
   }
 }

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