From 11368be4ac24108f18b1965162ad27f207c074f9 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Wed, 3 Aug 2022 01:43:26 -0700 Subject: [PATCH] Correctly report errors thrown by CommandLinePathFactory#create. 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 #16022. PiperOrigin-RevId: 464997904 Change-Id: I87fd5eaeb562d849bb830d68f1bfbbf06f6e0ea9 --- .../lib/runtime/CommandLinePathFactory.java | 20 ++++++++----- .../runtime/CommandLinePathFactoryTest.java | 30 +++++++++---------- 2 files changed, 26 insertions(+), 24 deletions(-) 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 index 3726ddac64fc2a..f6a2d196c94a58 100644 --- 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 /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 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 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 index 58276e928c146f..be20be0e03779d 100644 --- 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