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

8 files changed, 22 insertions, 21 deletions

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 2e29fab1a4..26d0eb9775 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 @@ public class CommandLines {
           ++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
index 462fe8278c..b4b3d028d0 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 @@ package com.google.devtools.build.lib.actions;
  * <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 0582a93a3f..43ea957491 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.ArtifactExpander;
 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 @@ public final class CustomCommandLine extends CommandLine {
       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 @@ public final class CustomCommandLine extends CommandLine {
       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 @@ public final class CustomCommandLine extends CommandLine {
         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 74509858d9..493f249532 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.BaseSpawn;
 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.ParamFileInfo;
 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 @@ public class SpawnAction extends AbstractAction implements ExecutionInfoSpecifie
           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 8ab8e3bd41..90343eba5d 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.ActionAnalysisMetadata;
 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 @@ public class SkylarkActionFactory implements SkylarkActionFactoryApi {
         throws EvalException {
       if (formatStr != null
           && skylarkSemantics.incompatibleDisallowOldStyleArgsAdd()
-          && !CommandLineItemSimpleFormatter.isValid(formatStr)) {
+          && !SingleStringArgFormatter.isValid(formatStr)) {
         throw new EvalException(
             null,
             String.format(
@@ -798,7 +798,7 @@ public class SkylarkActionFactory implements SkylarkActionFactoryApi {
       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 a67223a98e..3e44975ca7 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.ActionKeyContext;
 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 @@ public class SkylarkCustomCommandLine extends CommandLine {
 
     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
index d666c7a629..2c042a66c2 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 e55829d038..d3568b2017 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 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
         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