Skip to content

Commit cdace8f

Browse files
fmeumcopybara-github
fmeum
authored and
copybara-github
committed
Fail early on certain invalid labels in module files
The following validation checks were not enforced due to backwards compatibility concerns, but ended up crashing Bazel when invalid labels made it into the lockfile, which is enabled by default. We might as well enable them now: * Fail if a label passed to `use_extension` is not valid * Fail if a label passed to the `patches` attribute of an override is not valid or points to a repo other than the main repo Work towards #21845 Closes #22487. PiperOrigin-RevId: 636255834 Change-Id: I55dda374cd1716e514c4d78642479b136fd3ad43
1 parent 72c8ae5 commit cdace8f

File tree

7 files changed

+96
-40
lines changed

7 files changed

+96
-40
lines changed

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,15 @@
1818
import com.google.auto.value.AutoValue;
1919
import com.google.common.collect.ImmutableList;
2020
import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
21+
import com.google.devtools.build.lib.cmdline.Label;
2122

2223
/** Specifies that a module should be retrieved from an archive. */
2324
@AutoValue
2425
public abstract class ArchiveOverride implements NonRegistryOverride {
2526

2627
public static ArchiveOverride create(
2728
ImmutableList<String> urls,
28-
ImmutableList<Object> patches,
29+
ImmutableList<Label> patches,
2930
ImmutableList<String> patchCmds,
3031
String integrity,
3132
String stripPrefix,
@@ -38,7 +39,7 @@ public static ArchiveOverride create(
3839
public abstract ImmutableList<String> getUrls();
3940

4041
/** The labels of patches to apply after extracting the archive. */
41-
public abstract ImmutableList<Object> getPatches();
42+
public abstract ImmutableList<Label> getPatches();
4243

4344
/** The patch commands to execute after extracting the archive. Should be a list of commands. */
4445
public abstract ImmutableList<String> getPatchCmds();

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.google.common.base.Strings;
1919
import com.google.common.collect.ImmutableList;
2020
import com.google.common.collect.ImmutableMap;
21+
import com.google.devtools.build.lib.cmdline.Label;
2122
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2223
import net.starlark.java.eval.StarlarkInt;
2324

@@ -54,7 +55,7 @@ public ArchiveRepoSpecBuilder setStripPrefix(String stripPrefix) {
5455
}
5556

5657
@CanIgnoreReturnValue
57-
public ArchiveRepoSpecBuilder setPatches(ImmutableList<Object> patches) {
58+
public ArchiveRepoSpecBuilder setPatches(ImmutableList<Label> patches) {
5859
attrBuilder.put("patches", patches);
5960
return this;
6061
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,15 @@
1818
import com.google.auto.value.AutoValue;
1919
import com.google.common.collect.ImmutableList;
2020
import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
21+
import com.google.devtools.build.lib.cmdline.Label;
2122

2223
/** Specifies that a module should be retrieved from a Git repository. */
2324
@AutoValue
2425
public abstract class GitOverride implements NonRegistryOverride {
2526
public static GitOverride create(
2627
String remote,
2728
String commit,
28-
ImmutableList<Object> patches,
29+
ImmutableList<Label> patches,
2930
ImmutableList<String> patchCmds,
3031
int patchStrip,
3132
boolean initSubmodules,
@@ -41,7 +42,7 @@ public static GitOverride create(
4142
public abstract String getCommit();
4243

4344
/** The labels of patches to apply after fetching from Git. */
44-
public abstract ImmutableList<Object> getPatches();
45+
public abstract ImmutableList<Label> getPatches();
4546

4647
/** The patch commands to execute after fetching from Git. Should be a list of commands. */
4748
public abstract ImmutableList<String> getPatchCmds();

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

1818
import com.google.common.collect.ImmutableMap;
19+
import com.google.devtools.build.lib.cmdline.Label;
1920
import com.google.errorprone.annotations.CanIgnoreReturnValue;
2021
import java.util.List;
2122

@@ -71,7 +72,7 @@ public GitRepoSpecBuilder setStripPrefix(String stripPrefix) {
7172
}
7273

7374
@CanIgnoreReturnValue
74-
public GitRepoSpecBuilder setPatches(List<Object> patches) {
75+
public GitRepoSpecBuilder setPatches(List<Label> patches) {
7576
return setAttr("patches", patches);
7677
}
7778

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

Lines changed: 42 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515

1616
package com.google.devtools.build.lib.bazel.bzlmod;
1717

18-
import static com.google.common.collect.ImmutableList.toImmutableList;
1918

2019
import com.google.common.annotations.VisibleForTesting;
2120
import com.google.common.collect.ImmutableCollection;
@@ -470,7 +469,8 @@ public ModuleExtensionProxy useExtension(
470469
return new ModuleExtensionProxy(newUsageBuilder, proxyBuilder);
471470
}
472471

473-
private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile) {
472+
private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile)
473+
throws EvalException {
474474
// Normalize the label by parsing and stringifying it with a repo mapping that preserves the
475475
// apparent repository name, except that a reference to the main repository via the empty
476476
// repo name is translated to using the module repo name. This is necessary as
@@ -480,45 +480,49 @@ private String normalizeLabelString(InterimModule.Builder module, String rawExte
480480
// ownName can't change anymore as calling module() after this results in an error.
481481
String ownName = module.getRepoName().orElse(module.getName());
482482
RepositoryName ownRepoName = RepositoryName.createUnvalidated(ownName);
483+
ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
484+
if (module.getKey().equals(ModuleKey.ROOT)) {
485+
repoMapping = ImmutableMap.of("", ownRepoName);
486+
}
487+
Label label;
483488
try {
484-
ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
485-
if (module.getKey().equals(ModuleKey.ROOT)) {
486-
repoMapping = ImmutableMap.of("", ownRepoName);
487-
}
488-
Label label =
489+
label =
489490
Label.parseWithPackageContext(
490491
rawExtensionBzlFile,
491492
Label.PackageContext.of(
492493
PackageIdentifier.create(ownRepoName, PathFragment.EMPTY_FRAGMENT),
493494
RepositoryMapping.createAllowingFallback(repoMapping)));
494-
// Skip over the leading "@" of the unambiguous form.
495-
return label.getUnambiguousCanonicalForm().substring(1);
496-
} catch (LabelSyntaxException ignored) {
497-
// Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
498-
// let the extension fail when evaluated.
499-
return rawExtensionBzlFile;
495+
} catch (LabelSyntaxException e) {
496+
throw Starlark.errorf("invalid label \"%s\": %s", rawExtensionBzlFile, e.getMessage());
500497
}
498+
// Skip over the leading "@" of the unambiguous form.
499+
return label.getUnambiguousCanonicalForm().substring(1);
501500
}
502501

503-
/**
504-
* Returns a {@link Label} when the given string is a valid label, otherwise the string itself.
505-
*/
506-
private Object parseOverrideLabelAttribute(InterimModule.Builder module, String rawLabel) {
502+
private Label convertAndValidatePatchLabel(InterimModule.Builder module, String rawLabel)
503+
throws EvalException {
507504
RepositoryMapping repoMapping =
508505
RepositoryMapping.create(
509506
ImmutableMap.<String, RepositoryName>builder()
510507
.put("", RepositoryName.MAIN)
511508
.put(module.getRepoName().orElse(module.getName()), RepositoryName.MAIN)
512509
.buildKeepingLast(),
513510
RepositoryName.MAIN);
511+
Label label;
514512
try {
515-
return Label.parseWithPackageContext(
516-
rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
513+
label =
514+
Label.parseWithPackageContext(
515+
rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
517516
} catch (LabelSyntaxException e) {
518-
// Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
519-
// let the module repo fail when fetched.
520-
return rawLabel;
517+
throw Starlark.errorf("invalid label \"%s\" in 'patches': %s", rawLabel, e.getMessage());
521518
}
519+
if (!label.getRepository().isVisible()) {
520+
throw Starlark.errorf(
521+
"invalid label in 'patches': only patches in the main repository can be applied, not from"
522+
+ " '@%s'",
523+
label.getRepository().getName());
524+
}
525+
return label;
522526
}
523527

524528
@StarlarkBuiltin(name = "module_extension_proxy", documented = false)
@@ -811,16 +815,18 @@ public void singleVersionOverride(
811815
try {
812816
parsedVersion = Version.parse(version);
813817
} catch (ParseException e) {
814-
throw new EvalException("Invalid version in single_version_override()", e);
818+
throw Starlark.errorf("Invalid version in single_version_override(): %s", version);
819+
}
820+
ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
821+
for (String patch : Sequence.cast(patches, String.class, "patches")) {
822+
patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
815823
}
816824
context.addOverride(
817825
moduleName,
818826
SingleVersionOverride.create(
819827
parsedVersion,
820828
registry,
821-
Sequence.cast(patches, String.class, "patches").stream()
822-
.map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
823-
.collect(toImmutableList()),
829+
patchesBuilder.build(),
824830
Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
825831
patchStrip.toInt("single_version_override.patch_strip")));
826832
}
@@ -961,13 +967,15 @@ public void archiveOverride(
961967
urls instanceof String string
962968
? ImmutableList.of(string)
963969
: Sequence.cast(urls, String.class, "urls").getImmutableList();
970+
ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
971+
for (String patch : Sequence.cast(patches, String.class, "patches")) {
972+
patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
973+
}
964974
context.addOverride(
965975
moduleName,
966976
ArchiveOverride.create(
967977
urlList,
968-
Sequence.cast(patches, String.class, "patches").stream()
969-
.map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
970-
.collect(toImmutableList()),
978+
patchesBuilder.build(),
971979
Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
972980
integrity,
973981
stripPrefix,
@@ -1053,14 +1061,16 @@ public void gitOverride(
10531061
ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "git_override()");
10541062
context.setNonModuleCalled();
10551063
validateModuleName(moduleName);
1064+
ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
1065+
for (String patch : Sequence.cast(patches, String.class, "patches")) {
1066+
patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
1067+
}
10561068
context.addOverride(
10571069
moduleName,
10581070
GitOverride.create(
10591071
remote,
10601072
commit,
1061-
Sequence.cast(patches, String.class, "patches").stream()
1062-
.map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
1063-
.collect(toImmutableList()),
1073+
patchesBuilder.build(),
10641074
Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
10651075
patchStrip.toInt("git_override.patch_strip"),
10661076
initSubmodules,

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import com.google.auto.value.AutoValue;
1919
import com.google.common.collect.ImmutableList;
20+
import com.google.devtools.build.lib.cmdline.Label;
2021

2122
/**
2223
* Specifies that the module should:
@@ -33,7 +34,7 @@ public abstract class SingleVersionOverride implements RegistryOverride {
3334
public static SingleVersionOverride create(
3435
Version version,
3536
String registry,
36-
ImmutableList<Object> patches,
37+
ImmutableList<Label> patches,
3738
ImmutableList<String> patchCmds,
3839
int patchStrip) {
3940
return new AutoValue_SingleVersionOverride(version, registry, patches, patchCmds, patchStrip);
@@ -49,7 +50,7 @@ public static SingleVersionOverride create(
4950
public abstract String getRegistry();
5051

5152
/** The labels of patches to apply after retrieving per the registry. */
52-
public abstract ImmutableList<Object> getPatches();
53+
public abstract ImmutableList<Label> getPatches();
5354

5455
/**
5556
* The patch commands to execute after retrieving per the registry. Should be a list of commands.

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1611,4 +1611,45 @@ public void testRegisterToolchains_singlePackageRestriction() throws Exception {
16111611
ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
16121612
assertThat(result.hasError()).isFalse();
16131613
}
1614+
1615+
@Test
1616+
public void testInvalidRepoInPatches() throws Exception {
1617+
scratch.overwriteFile(
1618+
rootDirectory.getRelative("MODULE.bazel").getPathString(),
1619+
"module(name='aaa')",
1620+
"single_version_override(",
1621+
" module_name = 'bbb',",
1622+
" version = '1.0',",
1623+
" patches = ['@unknown_repo//:patch.bzl'],",
1624+
")");
1625+
1626+
reporter.removeHandler(failFastHandler);
1627+
EvaluationResult<RootModuleFileValue> result =
1628+
evaluator.evaluate(
1629+
ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
1630+
assertThat(result.hasError()).isTrue();
1631+
1632+
assertContainsEvent(
1633+
"Error in single_version_override: invalid label in 'patches': only patches in the main"
1634+
+ " repository can be applied, not from '@unknown_repo'");
1635+
}
1636+
1637+
@Test
1638+
public void testInvalidUseExtensionLabel() throws Exception {
1639+
scratch.overwriteFile(
1640+
rootDirectory.getRelative("MODULE.bazel").getPathString(),
1641+
"module(name='aaa')",
1642+
"use_extension('@foo/bar:extensions.bzl', 'my_ext')");
1643+
1644+
reporter.removeHandler(failFastHandler);
1645+
EvaluationResult<RootModuleFileValue> result =
1646+
evaluator.evaluate(
1647+
ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
1648+
assertThat(result.hasError()).isTrue();
1649+
1650+
assertContainsEvent(
1651+
"Error in use_extension: invalid label \"@foo/bar:extensions.bzl\": invalid repository"
1652+
+ " name 'foo/bar:extensions.bzl': repo names may contain only A-Z, a-z, 0-9, '-',"
1653+
+ " '_', '.' and '~' and must not start with '~'");
1654+
}
16141655
}

0 commit comments

Comments
 (0)