Ignore invalid entries in the marker file

Especially due to #23127, older marker files may contain entries that don't even parse correctly. Instead of crashing Bazel, we should just ignore such entries.

Fixes #23322.

Author: Wyverald

src/main/java/com/google/devtools/build/lib/rules/repository/RepoRecordedInput.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.actions.FileValue;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.skyframe.ActionEnvironmentFunction;
@@ -76,8 +77,10 @@ public abstract static class Parser {
 
     /**
      * Parses a recorded input from the post-colon substring that identifies the specific input: for
-     * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}.
+     * example, the {@code MY_ENV_VAR} part of {@code ENV:MY_ENV_VAR}. Returns null if the parsed
+     * part is invalid.
      */
+    @Nullable
     public abstract RepoRecordedInput parse(String s);
   }
 
@@ -95,6 +98,9 @@ public abstract static class Parser {
   @Nullable
   public static RepoRecordedInput parse(String s) {
     List<String> parts = Splitter.on(':').limit(2).splitToList(s);
+    if (parts.size() < 2) {
+      return null;
+    }
     for (Parser parser :
         new Parser[] {
           File.PARSER, Dirents.PARSER, DirTree.PARSER, EnvVar.PARSER, RecordedRepoMapping.PARSER
@@ -213,12 +219,12 @@ public final String toString() {
       return repoName().map(repoName -> repoName + "//" + path()).orElse(path().toString());
     }
 
-    public static RepoCacheFriendlyPath parse(String s) {
+    public static RepoCacheFriendlyPath parse(String s) throws LabelSyntaxException {
       if (LabelValidator.isAbsolute(s)) {
         int doubleSlash = s.indexOf("//");
         int skipAts = s.startsWith("@@") ? 2 : s.startsWith("@") ? 1 : 0;
         return createInsideWorkspace(
-            RepositoryName.createUnvalidated(s.substring(skipAts, doubleSlash)),
+            RepositoryName.create(s.substring(skipAts, doubleSlash)),
             PathFragment.create(s.substring(doubleSlash + 2)));
       }
       return createOutsideWorkspace(PathFragment.create(s));
@@ -262,9 +268,15 @@ public String getPrefix() {
             return "FILE";
           }
 
+          @Nullable
           @Override
           public RepoRecordedInput parse(String s) {
-            return new File(RepoCacheFriendlyPath.parse(s));
+            try {
+              return new File(RepoCacheFriendlyPath.parse(s));
+            } catch (LabelSyntaxException e) {
+              // ignore malformed input
+              return null;
+            }
           }
         };
 
@@ -354,9 +366,15 @@ public String getPrefix() {
             return "DIRENTS";
           }
 
+          @Nullable
           @Override
           public RepoRecordedInput parse(String s) {
-            return new Dirents(RepoCacheFriendlyPath.parse(s));
+            try {
+              return new Dirents(RepoCacheFriendlyPath.parse(s));
+            } catch (LabelSyntaxException e) {
+              // Ignore malformed input.
+              return null;
+            }
           }
         };
 
@@ -437,9 +455,15 @@ public String getPrefix() {
             return "DIRTREE";
           }
 
+          @Nullable
           @Override
           public RepoRecordedInput parse(String s) {
-            return new DirTree(RepoCacheFriendlyPath.parse(s));
+            try {
+              return new DirTree(RepoCacheFriendlyPath.parse(s));
+            } catch (LabelSyntaxException e) {
+              // ignore malformed input
+              return null;
+            }
           }
         };
 
@@ -573,11 +597,16 @@ public String getPrefix() {
             return "REPO_MAPPING";
           }
 
+          @Nullable
           @Override
           public RepoRecordedInput parse(String s) {
             List<String> parts = Splitter.on(',').limit(2).splitToList(s);
-            return new RecordedRepoMapping(
-                RepositoryName.createUnvalidated(parts.get(0)), parts.get(1));
+            try {
+              return new RecordedRepoMapping(RepositoryName.create(parts.get(0)), parts.get(1));
+            } catch (LabelSyntaxException | IndexOutOfBoundsException e) {
+              // ignore malformed input
+              return null;
+            }
           }
         };
 
@@ -632,9 +661,14 @@ public boolean isUpToDate(
         throws InterruptedException {
       RepositoryMappingValue repoMappingValue =
           (RepositoryMappingValue) env.getValue(getSkyKey(directories));
-      return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE
-          && RepositoryName.createUnvalidated(oldValue)
-              .equals(repoMappingValue.getRepositoryMapping().get(apparentName));
+      try {
+        return repoMappingValue != RepositoryMappingValue.NOT_FOUND_VALUE
+            && RepositoryName.create(oldValue)
+                .equals(repoMappingValue.getRepositoryMapping().get(apparentName));
+      } catch (LabelSyntaxException e) {
+        // ignore malformed old value
+        return false;
+      }
     }
   }
 }

src/test/shell/bazel/starlark_repository_test.sh

@@ -2864,6 +2864,50 @@ EOF
   expect_log "I see: something"
 }
 
+function test_bad_marker_file_ignored() {
+  # when reading a file in another repo, we should watch it.
+  local outside_dir="${TEST_TMPDIR}/outside_dir"
+  mkdir -p "${outside_dir}"
+  echo nothing > ${outside_dir}/data.txt
+
+  create_new_workspace
+  cat > $(setup_module_dot_bazel) <<EOF
+foo = use_repo_rule("//:r.bzl", "foo")
+foo(name = "foo")
+bar = use_repo_rule("//:r.bzl", "bar")
+bar(name = "bar", data = "nothing")
+EOF
+  touch BUILD
+  cat > r.bzl <<EOF
+def _foo(rctx):
+  rctx.file("BUILD", "filegroup(name='foo')")
+  # intentionally grab a file that's not directly addressable by a label
+  otherfile = rctx.path(Label("@bar//subpkg:BUILD")).dirname.dirname.get_child("data.txt")
+  print("I see: " + rctx.read(otherfile))
+foo=repository_rule(_foo)
+def _bar(rctx):
+  rctx.file("subpkg/BUILD")
+  rctx.file("data.txt", rctx.attr.data)
+bar=repository_rule(_bar, attrs={"data":attr.string()})
+EOF
+
+  bazel build @foo >& $TEST_log || fail "expected bazel to succeed"
+  expect_log "I see: nothing"
+
+  local marker_file=$(bazel info output_base)/external/@+_repo_rules+foo.marker
+  # the marker file for this repo should contain a reference to "@@+_repo_rules+bar". Mangle that.
+  sed -i'' -e 's/@@+_repo_rules+bar/@@LOL@@LOL/g' ${marker_file}
+  # add some more nonsensical lines for good measure...
+  echo 'BLAH:BLEH:BLOO BLEE' > ${marker_file}
+  echo 'no colons at all' > ${marker_file}
+  echo 'REPO_MAPPING:bad**bad,worse**worse gaaa'
+  echo 'REPO_MAPPING:bad,worse worst**worst'
+
+  # Running Bazel again shouldn't crash.
+  bazel shutdown
+  bazel build @foo >& $TEST_log || fail "expected bazel to succeed"
+}
+
 function test_file_watching_in_undefined_repo() {
   create_new_workspace
   cat > $(setup_module_dot_bazel) <<EOF