Fail early when writing a lockfile with invalid labels

Today, a repo rule's attributes might contain invalid labels because of unresolved apparent repo names. This could be due to either module extensions generating bad repo definitions, or users providing e.g. bad labels to `patches` attribute of overrides. This causes us to write a label like `@@[unknown repo 'foo' requested from @@]//:bar` into the lockfile, which will cause Bazel to crash on a subsequent attempt to parse the lockfile.

This PR fixes it so that we throw an error at the time of writing the lockfile, instead of writing a corrupted lockfile that users can't recover from unless they manually delete it. In the unlikely event that we do have a corrupted lockfile, we also no longer crash, but give a helpful error message instead.

Fixes #21845.

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/AttributeValuesAdapter.java

@@ -18,12 +18,14 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
 import com.google.gson.JsonArray;
 import com.google.gson.JsonElement;
 import com.google.gson.JsonNull;
 import com.google.gson.JsonObject;
+import com.google.gson.JsonParseException;
 import com.google.gson.JsonParser;
 import com.google.gson.JsonPrimitive;
 import com.google.gson.TypeAdapter;
@@ -69,30 +71,36 @@ public AttributeValues read(JsonReader in) throws IOException {
    * Starlark Object Types Bool Integer String Label List (Int, label, string) Dict (String,list) &
    * (Label, String)
    */
-  private JsonElement serializeObject(Object obj) {
+  private JsonElement serializeObject(Object obj) throws IOException {
     if (obj.equals(Starlark.NONE)) {
       return JsonNull.INSTANCE;
-    } else if (obj instanceof Boolean) {
-      return new JsonPrimitive((Boolean) obj);
-    } else if (obj instanceof StarlarkInt) {
+    } else if (obj instanceof Boolean bool) {
+      return new JsonPrimitive(bool);
+    } else if (obj instanceof StarlarkInt i) {
       try {
-        return new JsonPrimitive(((StarlarkInt) obj).toInt("serialization into the lockfile"));
+        return new JsonPrimitive(i.toInt("serialization into the lockfile"));
       } catch (EvalException e) {
         throw new IllegalArgumentException("Unable to parse StarlarkInt to Integer: " + e);
       }
-    } else if (obj instanceof String || obj instanceof Label) {
-      return new JsonPrimitive(serializeObjToString(obj));
-    } else if (obj instanceof Dict) {
+    } else if (obj instanceof String s) {
+      return new JsonPrimitive(serializeString(s));
+    } else if (obj instanceof Label label) {
+      return new JsonPrimitive(serializeLabel(label));
+    } else if (obj instanceof Dict<?, ?> dict) {
       JsonObject jsonObject = new JsonObject();
-      for (Map.Entry<?, ?> entry : ((Dict<?, ?>) obj).entrySet()) {
-        jsonObject.add(serializeObjToString(entry.getKey()), serializeObject(entry.getValue()));
+      for (Map.Entry<?, ?> entry : dict.entrySet()) {
+        String key =
+            entry.getKey() instanceof Label label
+                ? serializeLabel(label)
+                : serializeString((String) entry.getKey());
+        jsonObject.add(key, serializeObject(entry.getValue()));
       }
       return jsonObject;
-    } else if (obj instanceof Iterable) {
+    } else if (obj instanceof Iterable<?> iter) {
       // ListType supports any kind of Iterable, including Tuples and StarlarkLists. All of them
       // are converted to an equivalent StarlarkList during deserialization.
       JsonArray jsonArray = new JsonArray();
-      for (Object item : (Iterable<?>) obj) {
+      for (Object item : iter) {
         jsonArray.add(serializeObject(item));
       }
       return jsonArray;
@@ -128,38 +136,38 @@ private Object deserializeObject(JsonElement json) {
       for (JsonElement item : jsonArray) {
         list.add(deserializeObject(item));
       }
-      return StarlarkList.copyOf(Mutability.IMMUTABLE, list);
+      return StarlarkList.immutableCopyOf(list);
     } else {
       throw new IllegalArgumentException("Unsupported JSON element: " + json);
     }
   }
 
   @VisibleForTesting static final String STRING_ESCAPE_SEQUENCE = "'";
 
+  private String serializeLabel(Label label) throws IOException {
+    if (!label.getRepository().isVisible()) {
+      throw new IOException("invalid label found in repo attributes: " + label);
+    }
+    String s = label.getUnambiguousCanonicalForm();
+    Preconditions.checkState(s.startsWith("@@"));
+    return s;
+  }
+
   /**
-   * Serializes an object (Label or String) to String. A label is converted to a String as it is. A
-   * String that looks like a label is escaped so that it can be differentiated from a label when
-   * deserializing, otherwise it is emitted as is.
-   *
-   * @param obj String or Label
-   * @return serialized object
+   * Serializes a string. A string that looks like a label is escaped so that it can be
+   * differentiated from a label when deserializing (since labels are also serialized as strings),
+   * otherwise it is emitted as is.
    */
-  private String serializeObjToString(Object obj) {
-    if (obj instanceof Label) {
-      String labelString = ((Label) obj).getUnambiguousCanonicalForm();
-      Preconditions.checkState(labelString.startsWith("@@"));
-      return labelString;
-    }
-    String string = (String) obj;
+  private String serializeString(String s) {
     // Strings that start with "@@" need to be escaped to avoid being interpreted as a label. We
     // escape by wrapping the string in the escape sequence and strip one layer of this sequence
     // during deserialization, so strings that happen to already start and end with the escape
     // sequence also have to be escaped.
-    if (string.startsWith("@@")
-        || (string.startsWith(STRING_ESCAPE_SEQUENCE) && string.endsWith(STRING_ESCAPE_SEQUENCE))) {
-      return STRING_ESCAPE_SEQUENCE + string + STRING_ESCAPE_SEQUENCE;
+    if (s.startsWith("@@")
+        || (s.startsWith(STRING_ESCAPE_SEQUENCE) && s.endsWith(STRING_ESCAPE_SEQUENCE))) {
+      return STRING_ESCAPE_SEQUENCE + s + STRING_ESCAPE_SEQUENCE;
     }
-    return string;
+    return s;
   }
 
   /**
@@ -172,7 +180,12 @@ private String serializeObjToString(Object obj) {
   private Object deserializeStringToObject(String value) {
     // A string represents a label if and only if it starts with "@@".
     if (value.startsWith("@@")) {
-      return Label.parseCanonicalUnchecked(value);
+      try {
+        return Label.parseCanonical(value);
+      } catch (LabelSyntaxException e) {
+        throw new JsonParseException(
+            String.format("error parsing label \"%s\" from the lockfile", value), e);
+      }
     }
     // Strings that start and end with the escape sequence always require one layer to be stripped.
     if (value.startsWith(STRING_ESCAPE_SEQUENCE) && value.endsWith(STRING_ESCAPE_SEQUENCE)) {

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD

@@ -105,8 +105,11 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/bazel/repository:repository_options",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
+        "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
         "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/protobuf:failure_details_java_proto",
         "//third_party:flogger",
+        "//third_party:gson",
         "//third_party:guava",
         "//third_party:jsr305",
     ],

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunction.java

@@ -36,6 +36,7 @@
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
+import com.google.gson.JsonParseException;
 import com.google.gson.JsonSyntaxException;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -85,7 +86,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
 
     try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.BZLMOD, "parse lockfile")) {
       return getLockfileValue(lockfilePath, rootDirectory);
-    } catch (IOException | JsonSyntaxException | NullPointerException e) {
+    } catch (IOException | JsonParseException | NullPointerException e) {
       throw new BazelLockfileFunctionException(
           ExternalDepsException.withMessage(
               Code.BAD_MODULE,

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileModule.java

@@ -26,11 +26,16 @@
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.runtime.BlazeModule;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.server.FailureDetails.ExternalRepository;
+import com.google.devtools.build.lib.server.FailureDetails.ExternalRepository.Code;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.util.AbruptExitException;
+import com.google.devtools.build.lib.util.DetailedExitCode;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.Root;
 import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.gson.JsonIOException;
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashMap;
@@ -83,6 +88,9 @@ public void afterCommand() throws AbruptExitException {
               + " Try deleting it and rerun the build.",
           e.getMessage());
       return;
+    } finally {
+      this.moduleResolutionEvent = null;
+      this.extensionResolutionEventsMap.clear();
     }
 
     // Write the new value to the file, but only if needed. This is not just a performance
@@ -92,8 +100,6 @@ public void afterCommand() throws AbruptExitException {
     if (!newLockfile.equals(oldLockfile)) {
       updateLockfile(workspaceRoot, newLockfile);
     }
-    this.moduleResolutionEvent = null;
-    this.extensionResolutionEventsMap.clear();
   }
 
   /**
@@ -201,7 +207,8 @@ private boolean shouldKeepExtension(
    * @param workspaceRoot Root of the workspace where the lockfile is located
    * @param updatedLockfile The updated lockfile data to save
    */
-  public static void updateLockfile(Path workspaceRoot, BazelLockFileValue updatedLockfile) {
+  public static void updateLockfile(Path workspaceRoot, BazelLockFileValue updatedLockfile)
+      throws AbruptExitException {
     RootedPath lockfilePath =
         RootedPath.toRootedPath(Root.fromPath(workspaceRoot), LabelConstants.MODULE_LOCKFILE_NAME);
     try {
@@ -216,9 +223,17 @@ public static void updateLockfile(Path workspaceRoot, BazelLockFileValue updated
                       workspaceRoot)
                   .toJson(updatedLockfile)
               + "\n");
-    } catch (IOException e) {
+    } catch (JsonIOException | IOException e) {
       logger.atSevere().withCause(e).log(
           "Error while updating MODULE.bazel.lock file: %s", e.getMessage());
+      throw new AbruptExitException(
+          DetailedExitCode.of(
+              FailureDetail.newBuilder()
+                  .setMessage("Error while updating MODULE.bazel.lock file: " + e.getMessage())
+                  .setExternalRepository(
+                      ExternalRepository.newBuilder().setCode(Code.LOCK_FILE_ERROR))
+                  .build()),
+          e);
     }
   }
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java

@@ -95,29 +95,25 @@ public ModuleKey read(JsonReader jsonReader) throws IOException {
         }
       };
 
-  public static final TypeAdapter<Label> LABEL_TYPE_ADAPTER =
-      new TypeAdapter<>() {
-        @Override
-        public void write(JsonWriter jsonWriter, Label label) throws IOException {
-          jsonWriter.value(label.getUnambiguousCanonicalForm());
-        }
-
-        @Override
-        public Label read(JsonReader jsonReader) throws IOException {
-          return Label.parseCanonicalUnchecked(jsonReader.nextString());
-        }
-      };
-
   public static final TypeAdapter<RepositoryName> REPOSITORY_NAME_TYPE_ADAPTER =
       new TypeAdapter<>() {
         @Override
         public void write(JsonWriter jsonWriter, RepositoryName repoName) throws IOException {
+          if (!repoName.isVisible()) {
+            throw new IOException("invalid repo name found while writing lockfile: " + repoName);
+          }
           jsonWriter.value(repoName.getName());
         }
 
         @Override
         public RepositoryName read(JsonReader jsonReader) throws IOException {
-          return RepositoryName.createUnvalidated(jsonReader.nextString());
+          String jsonString = jsonReader.nextString();
+          try {
+            return RepositoryName.create(jsonString);
+          } catch (LabelSyntaxException e) {
+            throw new JsonParseException(
+                String.format("error parsing repo name \"%s\" from the lockfile", jsonString), e);
+          }
         }
       };
 
@@ -498,7 +494,6 @@ private static GsonBuilder newGsonBuilder() {
         .registerTypeAdapterFactory(IMMUTABLE_SET)
         .registerTypeAdapterFactory(OPTIONAL)
         .registerTypeAdapterFactory(IMMUTABLE_TABLE)
-        .registerTypeAdapter(Label.class, LABEL_TYPE_ADAPTER)
         .registerTypeAdapter(RepositoryName.class, REPOSITORY_NAME_TYPE_ADAPTER)
         .registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER)
         .registerTypeAdapter(ModuleKey.class, MODULE_KEY_TYPE_ADAPTER)

src/main/protobuf/failure_details.proto

@@ -251,6 +251,7 @@ message ExternalRepository {
     BAD_DOWNLOADER_CONFIG = 2 [(metadata) = { exit_code: 2 }];
     REPOSITORY_MAPPING_RESOLUTION_FAILED = 3 [(metadata) = { exit_code: 37 }];
     CREDENTIALS_INIT_FAILURE = 4 [(metadata) = { exit_code: 2 }];
+    LOCK_FILE_ERROR = 5 [(metadata) = { exit_code: 37 }];
   }
   Code code = 1;
   // Additional data could include external repository names.

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD

@@ -71,6 +71,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
         "//src/main/java/com/google/devtools/build/lib/skyframe:skyframe_cluster",
         "//src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository",
+        "//src/main/java/com/google/devtools/build/lib/util:abrupt_exit_exception",
         "//src/main/java/com/google/devtools/build/lib/util:filetype",
         "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/vfs",

src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileFunctionTest.java

@@ -53,6 +53,7 @@
 import com.google.devtools.build.lib.starlarkbuildapi.repository.RepositoryBootstrap;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.util.AbruptExitException;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.FileStateKey;
 import com.google.devtools.build.lib.vfs.Root;
@@ -187,14 +188,18 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                         if (localOverrideHashes == null) {
                           return null;
                         }
-                        BazelLockFileModule.updateLockfile(
-                            rootDirectory,
-                            BazelLockFileValue.builder()
-                                .setModuleFileHash(key.moduleHash())
-                                .setFlags(flags)
-                                .setLocalOverrideHashes(localOverrideHashes)
-                                .setModuleDepGraph(key.depGraph())
-                                .build());
+                        try {
+                          BazelLockFileModule.updateLockfile(
+                              rootDirectory,
+                              BazelLockFileValue.builder()
+                                  .setModuleFileHash(key.moduleHash())
+                                  .setFlags(flags)
+                                  .setLocalOverrideHashes(localOverrideHashes)
+                                  .setModuleDepGraph(key.depGraph())
+                                  .build());
+                        } catch (AbruptExitException e) {
+                          throw new IllegalStateException(e);
+                        }
 
                         return new SkyValue() {};
                       }

src/test/py/bazel/bzlmod/bazel_lockfile_test.py

@@ -2114,6 +2114,50 @@ def testForceWatchingDirentsOutsideWorkspaceFails(self):
         'attempted to watch path outside workspace', '\n'.join(stderr)
     )
 
+  def testBadLabelInRepoAttrsDoesntCorruptLockfile(self):
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            'ext = use_extension("extension.bzl", "ext")',
+            'use_repo(ext, "repo")'
+        ],
+    )
+    self.ScratchFile('BUILD.bazel')
+    self.ScratchFile(
+        'extension.bzl',
+        [
+            'def _repo_rule_impl(ctx):',
+            '    ctx.file("BUILD", "filegroup(name=\'repo\')")',
+            'repo_rule = repository_rule(_repo_rule_impl,',
+            '    attrs={"label":attr.label()})',
+            '',
+            'def _ext_impl(ctx):',
+            '    repo_rule(name="repo", label="@other_repo")',
+            '    repo_rule(name="other_repo")',
+            'ext = module_extension(implementation=_ext_impl)',
+        ],
+    )
+
+    # As it is, the `label="@other_repo"` part is an invalid label, since the
+    # root module doesn't `use_repo()` `other_repo`.
+    # This build could succeed, but the lockfile shouldn't contain an
+    # unparsable label like
+    #   @@[unknown repo 'other_repo' requested from @@]//:other_repo
+    self.RunBazel(['build', '@repo'], allow_failure=True)
+    self.RunBazel(['shutdown'])
+
+    # rewrite the MODULE.bazel file to make the previously bad label a good one.
+    # If the lockfile contained the unparsable label, we wouldn't be able to
+    # recover here.
+    self.ScratchFile(
+        'MODULE.bazel',
+        [
+            'ext = use_extension("extension.bzl", "ext")',
+            'use_repo(ext, "repo", "other_repo")'
+        ],
+    )
+    self.RunBazel(['build', '@repo'])
+
 
 if __name__ == '__main__':
   absltest.main()