Use the single-string arg formatter for param file format.
This avoids bazel crashes for illegally formatted strings. Previously the code would assume that a correct string was passed with only minimal validation.
RELNOTES: None
PiperOrigin-RevId: 206012819
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
index 2e29fab..26d0eb9 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
@@ -164,9 +164,8 @@
++paramFileNameSuffix;
String paramArg =
- paramFileInfo
- .getFlagFormatString()
- .replaceFirst("%s", paramFileExecPath.getPathString());
+ SingleStringArgFormatter.format(
+ paramFileInfo.getFlagFormatString(), paramFileExecPath.getPathString());
arguments.addElement(paramArg);
cmdLineLength += paramArg.length() + 1;
paramFiles.add(
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java b/src/main/java/com/google/devtools/build/lib/actions/SingleStringArgFormatter.java
similarity index 98%
rename from src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java
rename to src/main/java/com/google/devtools/build/lib/actions/SingleStringArgFormatter.java
index 462fe82..b4b3d02 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatter.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SingleStringArgFormatter.java
@@ -19,7 +19,7 @@
* <p>This implementation is used in command line item expansions that use formatting. We use a
* custom implementation to improve performance and avoid GC.
*/
-public class CommandLineItemSimpleFormatter {
+public class SingleStringArgFormatter {
/**
* Returns true if the format string contains a single '%s'.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
index 0582a93..43ea957 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/CustomCommandLine.java
@@ -28,7 +28,7 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.CommandLineItem;
-import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
+import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
@@ -241,7 +241,7 @@
return new Builder().each(values);
}
- /** Each argument is formatted via {@link CommandLineItemSimpleFormatter#format}. */
+ /** Each argument is formatted via {@link SingleStringArgFormatter#format}. */
public static Builder format(@CompileTimeConstant String formatEach) {
return new Builder().format(formatEach);
}
@@ -261,7 +261,7 @@
private String beforeEach;
private String joinWith;
- /** Each argument is formatted via {@link CommandLineItemSimpleFormatter#format}. */
+ /** Each argument is formatted via {@link SingleStringArgFormatter#format}. */
public Builder format(@CompileTimeConstant String formatEach) {
Preconditions.checkNotNull(formatEach);
this.formatEach = formatEach;
@@ -404,8 +404,7 @@
if (hasFormatEach) {
String formatStr = (String) arguments.get(argi++);
for (int i = 0; i < count; ++i) {
- mutatedValues.set(
- i, CommandLineItemSimpleFormatter.format(formatStr, mutatedValues.get(i)));
+ mutatedValues.set(i, SingleStringArgFormatter.format(formatStr, mutatedValues.get(i)));
}
}
if (hasBeforeEach) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
index 7450985..493f249 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java
@@ -38,7 +38,6 @@
import com.google.devtools.build.lib.actions.CommandAction;
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
-import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
import com.google.devtools.build.lib.actions.CommandLines;
import com.google.devtools.build.lib.actions.CommandLines.CommandLineAndParamFileInfo;
import com.google.devtools.build.lib.actions.CommandLines.CommandLineLimits;
@@ -52,6 +51,7 @@
import com.google.devtools.build.lib.actions.ParameterFile;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
+import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnActionContext;
import com.google.devtools.build.lib.actions.SpawnResult;
@@ -1380,8 +1380,7 @@
Artifact paramFile = (Artifact) value;
String flagFormatString = (String) values[++i];
result.add(
- CommandLineItemSimpleFormatter.format(
- flagFormatString, paramFile.getExecPathString()));
+ SingleStringArgFormatter.format(flagFormatString, paramFile.getExecPathString()));
} else if (value instanceof CommandLine) {
CommandLine commandLine = (CommandLine) value;
if (artifactExpander != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
index 8ab8e3b..90343eb 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java
@@ -24,10 +24,10 @@
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.CommandLine;
-import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
import com.google.devtools.build.lib.actions.ParamFileInfo;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.RunfilesSupplier;
+import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.actions.extra.ExtraActionInfo;
import com.google.devtools.build.lib.actions.extra.SpawnInfo;
import com.google.devtools.build.lib.analysis.CommandHelper;
@@ -772,7 +772,7 @@
throws EvalException {
if (formatStr != null
&& skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()
- && !CommandLineItemSimpleFormatter.isValid(formatStr)) {
+ && !SingleStringArgFormatter.isValid(formatStr)) {
throw new EvalException(
null,
String.format(
@@ -798,7 +798,7 @@
if (isImmutable()) {
throw new EvalException(null, "cannot modify frozen value");
}
- if (!paramFileArg.contains("%s")) {
+ if (!SingleStringArgFormatter.isValid(paramFileArg)) {
throw new EvalException(
null,
"Invalid value for parameter \"param_file_arg\": Expected string with a single \"%s\"");
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
index a67223a..3e44975 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkCustomCommandLine.java
@@ -23,7 +23,7 @@
import com.google.devtools.build.lib.actions.CommandLine;
import com.google.devtools.build.lib.actions.CommandLineExpansionException;
import com.google.devtools.build.lib.actions.CommandLineItem;
-import com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter;
+import com.google.devtools.build.lib.actions.SingleStringArgFormatter;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.events.Location;
@@ -702,7 +702,7 @@
static Formatter get(Location location, SkylarkSemantics skylarkSemantics) {
return skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()
- ? CommandLineItemSimpleFormatter::format
+ ? SingleStringArgFormatter::format
: new LegacyFormatter(location);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatterTest.java b/src/test/java/com/google/devtools/build/lib/actions/SingleStringArgFormatterTest.java
similarity index 87%
rename from src/test/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatterTest.java
rename to src/test/java/com/google/devtools/build/lib/actions/SingleStringArgFormatterTest.java
index d666c7a..2c042a6 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/CommandLineItemSimpleFormatterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/SingleStringArgFormatterTest.java
@@ -14,17 +14,17 @@
package com.google.devtools.build.lib.actions;
import static com.google.common.truth.Truth.assertThat;
-import static com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter.format;
-import static com.google.devtools.build.lib.actions.CommandLineItemSimpleFormatter.isValid;
+import static com.google.devtools.build.lib.actions.SingleStringArgFormatter.format;
+import static com.google.devtools.build.lib.actions.SingleStringArgFormatter.isValid;
import com.google.devtools.build.lib.testutil.MoreAsserts;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Tests for {@link CommandLineItemSimpleFormatter} */
+/** Tests for {@link SingleStringArgFormatter} */
@RunWith(JUnit4.class)
-public class CommandLineItemSimpleFormatterTest {
+public class SingleStringArgFormatterTest {
@Test
public void testValidate() {
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index e55829d..d3568b2 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -2379,6 +2379,10 @@
ruleContext,
"Invalid value for parameter \"param_file_arg\": Expected string with a single \"%s\"",
"args = ruleContext.actions.args()\n" + "args.use_param_file('--file=')");
+ checkError(
+ ruleContext,
+ "Invalid value for parameter \"param_file_arg\": Expected string with a single \"%s\"",
+ "args = ruleContext.actions.args()\n" + "args.use_param_file('--file=%s%s')");
}
@Test
