Correctly report errors thrown by CommandLinePathFactory#create.

tjgq · copybara-github · commit 11368be4ac24 · 2022-08-03T01:44:47.000-07:00
This method is called while initializing the remote module [1]. Any exceptions not derived from java.io.IOException cause a silent server crash. [1] https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java;l=436;drc=f3211f00ae08746b5794ab01d404c32b43146aba Closes bazelbuild#16022. PiperOrigin-RevId: 464997904 Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandLinePathFactory.java
@@ -39,7 +39,14 @@
  * (e.g., {@code %workspace%/foo} becomes {@code </path/to/workspace>/foo}).
  */
 public final class CommandLinePathFactory {
-  private static final Pattern REPLACEMENT_PATTERN = Pattern.compile("^(%([a-z_]+)%/)?([^%].*)$");
+  /** An exception thrown while attempting to resolve a path. */
+  public static class CommandLinePathFactoryException extends IOException {
+    public CommandLinePathFactoryException(String message) {
+      super(message);
+    }
+  }
+
+  private static final Pattern REPLACEMENT_PATTERN = Pattern.compile("^(%([a-z_]+)%/+)?([^%].*)$");
 
   private static final Splitter PATH_SPLITTER = Splitter.on(File.pathSeparator);
 
@@ -78,20 +85,17 @@ public Path create(Map<String, String> env, String value) throws IOException {
     String rootName = matcher.group(2);
     PathFragment path = PathFragment.create(matcher.group(3));
     if (path.containsUplevelReferences()) {
-      throw new IllegalArgumentException(
+      throw new CommandLinePathFactoryException(
           String.format(
               Locale.US, "Path must not contain any uplevel references ('..'), got '%s'", value));
     }
 
     // Case 1: `path` is relative to a well-known root.
     if (!Strings.isNullOrEmpty(rootName)) {
-      // The regex above cannot check that `value` is not of form `%foo%//abc` (group 2 will be
-      // `foo` and group 3 will be `/abc`).
-      Preconditions.checkArgument(!path.isAbsolute());
-
       Path root = roots.get(rootName);
       if (root == null) {
-        throw new IllegalArgumentException(String.format(Locale.US, "Unknown root %s", rootName));
+        throw new CommandLinePathFactoryException(
+            String.format(Locale.US, "Unknown root %s", rootName));
       }
       return root.getRelative(path);
     }
@@ -108,7 +112,7 @@ public Path create(Map<String, String> env, String value) throws IOException {
     // flag is from?), we only allow relative paths with a single segment (i.e., no `/`) and treat
     // it as relative to the user's `PATH`.
     if (path.segmentCount() > 1) {
-      throw new IllegalArgumentException(
+      throw new CommandLinePathFactoryException(
           "Path must either be absolute or not contain any path separators");
     }
 
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java b/src/test/java/com/google/devtools/build/lib/runtime/CommandLinePathFactoryTest.java
@@ -19,6 +19,7 @@
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.runtime.CommandLinePathFactory.CommandLinePathFactoryException;
 import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
@@ -99,6 +100,9 @@ public void createWithNamedRoot() throws Exception {
         .isEqualTo(filesystem.getPath("/path/to/output/base/foo"));
     assertThat(factory.create(ImmutableMap.of(), "%output_base%/foo/bar"))
         .isEqualTo(filesystem.getPath("/path/to/output/base/foo/bar"));
+
+    assertThat(factory.create(ImmutableMap.of(), "%workspace%//foo//bar"))
+        .isEqualTo(filesystem.getPath("/path/to/workspace/foo/bar"));
   }
 
   @Test
@@ -108,9 +112,11 @@ public void pathLeakingOutsideOfRoot() {
             filesystem, ImmutableMap.of("a", filesystem.getPath("/path/to/a")));
 
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%a%/../foo"));
+        CommandLinePathFactoryException.class,
+        () -> factory.create(ImmutableMap.of(), "%a%/../foo"));
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%a%/b/../.."));
+        CommandLinePathFactoryException.class,
+        () -> factory.create(ImmutableMap.of(), "%a%/b/../.."));
   }
 
   @Test
@@ -120,29 +126,21 @@ public void unknownRoot() {
             filesystem, ImmutableMap.of("a", filesystem.getPath("/path/to/a")));
 
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%workspace%/foo"));
+        CommandLinePathFactoryException.class,
+        () -> factory.create(ImmutableMap.of(), "%workspace%/foo"));
     assertThrows(
-        IllegalArgumentException.class,
+        CommandLinePathFactoryException.class,
         () -> factory.create(ImmutableMap.of(), "%output_base%/foo"));
   }
 
-  @Test
-  public void rootWithDoubleSlash() {
-    CommandLinePathFactory factory =
-        new CommandLinePathFactory(
-            filesystem, ImmutableMap.of("a", filesystem.getPath("/path/to/a")));
-
-    assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "%a%//foo"));
-  }
-
   @Test
   public void relativePathWithMultipleSegments() {
     CommandLinePathFactory factory = new CommandLinePathFactory(filesystem, ImmutableMap.of());
 
-    assertThrows(IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "a/b"));
     assertThrows(
-        IllegalArgumentException.class, () -> factory.create(ImmutableMap.of(), "a/b/c/d"));
+        CommandLinePathFactoryException.class, () -> factory.create(ImmutableMap.of(), "a/b"));
+    assertThrows(
+        CommandLinePathFactoryException.class, () -> factory.create(ImmutableMap.of(), "a/b/c/d"));
   }
 
   @Test
