runfiles: rlocation() checks if arg is normalized

rlocation() now validates that the path argument
is normalized, i.e. contains none of:
- current directory references (".")
- uplevel references ("..")
- double slash ("//")

This helps avoiding a bug similar to https://github.com/bazelbuild/bazel/pull/5083.

See https://github.com/bazelbuild/bazel/pull/4460

Change-Id: Ia73fa2df1eae86fc7084162c24e144129672742d

Closes #5085.

Change-Id: Ia73fa2df1eae86fc7084162c24e144129672742d
PiperOrigin-RevId: 194074072

4 files changed, 72 insertions, 7 deletions

diff --git a/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java b/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java
index c342985500..f72be41872 100644
--- a/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java
+++ b/src/tools/runfiles/java/com/google/devtools/build/runfiles/Runfiles.java
@@ -100,12 +100,20 @@ public abstract class Runfiles {
    *
    * @param path runfiles-root-relative path of the runfile
    * @throws IllegalArgumentException if {@code path} fails validation, for example if it's null or
-   *     empty, or contains uplevel references
+   *     empty, or not normalized (contains "./", "../", or "//")
    */
   public final String rlocation(String path) {
     Util.checkArgument(path != null);
     Util.checkArgument(!path.isEmpty());
-    Util.checkArgument(!path.contains(".."), "path contains uplevel references: \"%s\"", path);
+    Util.checkArgument(
+        !path.startsWith("../")
+            && !path.contains("/..")
+            && !path.startsWith("./")
+            && !path.contains("/./")
+            && !path.endsWith("/.")
+            && !path.contains("//"),
+        "path is not normalized: \"%s\"",
+        path);
     Util.checkArgument(
         !path.startsWith("\\"), "path is absolute without a drive letter: \"%s\"", path);
     if (new File(path).isAbsolute()) {
diff --git a/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java b/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java
index af9e8d6ced..5c2eead374 100644
--- a/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java
+++ b/src/tools/runfiles/java/com/google/devtools/build/runfiles/RunfilesTest.java
@@ -61,7 +61,15 @@ public final class RunfilesTest {
     Runfiles r = Runfiles.create(ImmutableMap.of("RUNFILES_DIR", dir.toString()));
     assertRlocationArg(r, null, null);
     assertRlocationArg(r, "", null);
-    assertRlocationArg(r, "foo/..", "contains uplevel");
+    assertRlocationArg(r, "../foo", "is not normalized");
+    assertRlocationArg(r, "foo/..", "is not normalized");
+    assertRlocationArg(r, "foo/../bar", "is not normalized");
+    assertRlocationArg(r, "./foo", "is not normalized");
+    assertRlocationArg(r, "foo/.", "is not normalized");
+    assertRlocationArg(r, "foo/./bar", "is not normalized");
+    assertRlocationArg(r, "//foobar", "is not normalized");
+    assertRlocationArg(r, "foo//", "is not normalized");
+    assertRlocationArg(r, "foo//bar", "is not normalized");
     assertRlocationArg(r, "\\foo", "path is absolute without a drive letter");
   }
 
diff --git a/src/tools/runfiles/runfiles.cc b/src/tools/runfiles/runfiles.cc
index b1a870558b..801e44e374 100644
--- a/src/tools/runfiles/runfiles.cc
+++ b/src/tools/runfiles/runfiles.cc
@@ -42,6 +42,36 @@ using std::vector;
 
 namespace {
 
+bool starts_with(const string& s, const string& prefix) {
+  if (prefix.empty()) {
+    return true;
+  }
+  if (s.empty()) {
+    return false;
+  }
+  return s.find(prefix) == 0;
+}
+
+bool contains(const string& s, const string& substr) {
+  if (substr.empty()) {
+    return true;
+  }
+  if (s.empty()) {
+    return false;
+  }
+  return s.find(substr) != string::npos;
+}
+
+bool ends_with(const string& s, const string& suffix) {
+  if (suffix.empty()) {
+    return true;
+  }
+  if (s.empty()) {
+    return false;
+  }
+  return s.rfind(suffix) == s.size() - suffix.size();
+}
+
 class RunfilesImpl : public Runfiles {
  public:
   static Runfiles* Create(const string& argv0,
@@ -163,9 +193,10 @@ bool IsAbsolute(const string& path) {
     return false;
   }
   char c = path.front();
-  return (c == '/') || (path.size() >= 3 &&
-                        ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) &&
-                        path[1] == ':' && (path[2] == '\\' || path[2] == '/'));
+  return (c == '/' && (path.size() < 2 || path[1] != '/')) ||
+         (path.size() >= 3 &&
+          ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')) &&
+          path[1] == ':' && (path[2] == '\\' || path[2] == '/'));
 }
 
 string GetEnv(const string& key) {
@@ -184,7 +215,9 @@ string GetEnv(const string& key) {
 }
 
 string RunfilesImpl::Rlocation(const string& path) const {
-  if (path.empty() || path.find("..") != string::npos) {
+  if (path.empty() || starts_with(path, "../") || contains(path, "/..") ||
+      starts_with(path, "./") || contains(path, "/./") ||
+      ends_with(path, "/.") || contains(path, "//")) {
     return std::move(string());
   }
   if (IsAbsolute(path)) {
diff --git a/src/tools/runfiles/runfiles_test.cc b/src/tools/runfiles/runfiles_test.cc
index 9fe95aa19b..b15fc04a68 100644
--- a/src/tools/runfiles/runfiles_test.cc
+++ b/src/tools/runfiles/runfiles_test.cc
@@ -226,7 +226,15 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesRlocation) {
   EXPECT_EQ(r->Rlocation("foo"), "");
   EXPECT_EQ(r->Rlocation("foo/"), "");
   EXPECT_EQ(r->Rlocation("foo/bar"), "");
+  EXPECT_EQ(r->Rlocation("../foo"), "");
   EXPECT_EQ(r->Rlocation("foo/.."), "");
+  EXPECT_EQ(r->Rlocation("foo/../bar"), "");
+  EXPECT_EQ(r->Rlocation("./foo"), "");
+  EXPECT_EQ(r->Rlocation("foo/."), "");
+  EXPECT_EQ(r->Rlocation("foo/./bar"), "");
+  EXPECT_EQ(r->Rlocation("//foo"), "");
+  EXPECT_EQ(r->Rlocation("foo//"), "");
+  EXPECT_EQ(r->Rlocation("foo//bar"), "");
   EXPECT_EQ(r->Rlocation("/Foo"), "/Foo");
   EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo");
   EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo");
@@ -244,7 +252,15 @@ TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocation) {
   EXPECT_EQ(r->Rlocation("foo"), "whatever/foo");
   EXPECT_EQ(r->Rlocation("foo/"), "whatever/foo/");
   EXPECT_EQ(r->Rlocation("foo/bar"), "whatever/foo/bar");
+  EXPECT_EQ(r->Rlocation("../foo"), "");
   EXPECT_EQ(r->Rlocation("foo/.."), "");
+  EXPECT_EQ(r->Rlocation("foo/../bar"), "");
+  EXPECT_EQ(r->Rlocation("./foo"), "");
+  EXPECT_EQ(r->Rlocation("foo/."), "");
+  EXPECT_EQ(r->Rlocation("foo/./bar"), "");
+  EXPECT_EQ(r->Rlocation("//foo"), "");
+  EXPECT_EQ(r->Rlocation("foo//"), "");
+  EXPECT_EQ(r->Rlocation("foo//bar"), "");
   EXPECT_EQ(r->Rlocation("/Foo"), "/Foo");
   EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo");
   EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo");