Skip to content

Commit

Permalink
Correctly report errors thrown by CommandLinePathFactory#create.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tjgq committed Aug 2, 2022
1 parent e53162b commit 4b87cb6
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 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);

Expand Down Expand Up @@ -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);
}
Expand All @@ -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");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 4b87cb6

Please sign in to comment.