Escape analysistest failure message

Currently, the failure message does not output correctly if it contains sequences subject to shell variable expansion (e.g. `$1`) or the limit string used to denote the end of the message (in this case `\nEOF\n`). Those are quite plausible edge-cases, since analysistest targets might assert about the command-line of actions generated with `ctx.actions.run_shell` or the contents of shell files generated with `ctx.actions.write`.

This uses the approach of echoing by line and escaping characters as necessary for both versions. In the sh case, we can baskslash-escape every character. For Windows, `%` is escaped as `%%`. (Other special characters don't seem to cause trouble, though I'm probably missing relevant cases.)

The heredoc style `cat<<EOF\n[...]\nEOF\n` can have shell-escaping disabled by quoting the limit-string (`cat<<'EOF'`) or preceding it with a backslash (`cat<<\EOF`). But that still relies on choosing a limit string that doesn't occur in the contents, and there's no way (whether or not shell expansion is done) of escaping the limit string if it does. While one could choose a more obscure limit string, that would add another hidden implementation detail. Alternately, this could add logic to choose a limit string that doesn't occur in the message, but that seems unnecessarily complex.

Another approach would be to write the message to a file, then read it with cat/type. However, that requires either splitting the change between Starlark and Java code with separate release cycles (awkward), or doing something to insert the message file in the test's runfiles at the last minute (more complicated).

Adds some new test-cases which fail without this change.

Removes an unused return value to make the usage of this function a bit clearer.

bazelbuild/bazel-skylib#320 for the corresponding
change for unittest.

RELNOTES: None.
PiperOrigin-RevId: 400309738

src/main/java/com/google/devtools/build/lib/analysis/test/AnalysisTestActionBuilder.java

@@ -1,3 +1,4 @@
+
 // Copyright 2018 The Bazel Authors. All rights reserved.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -20,49 +21,50 @@
 import com.google.devtools.build.lib.util.OS;
 
 /**
- * Helper for writing test actions for analysis test rules. Analysis test rules are
- * restricted to disallow the rule implementation functions from registering actions themselves;
- * such rules register test success/failure via {@link AnalysisTestResultInfo}. This helper
- * registers the appropriate test script simulating success or failure of the test.
+ * Helper for writing test actions for analysis test rules. Analysis test rules are restricted to
+ * disallow the rule implementation functions from registering actions themselves; such rules
+ * register test success/failure via {@link AnalysisTestResultInfo}. This helper registers the
+ * appropriate test script simulating success or failure of the test.
  */
 public class AnalysisTestActionBuilder {
 
+  private AnalysisTestActionBuilder() {}
+
   /**
-   * Register and return an action to write a test script to the default executable location
-   * reflecting the given info object.
+   * Register an action to write a test script to the default executable location. The script should
+   * return exit status 0 if the test passed. It should print the failure message and return exit
+   * status 1 if the test failed.
    */
-  public static FileWriteAction writeAnalysisTestAction(
-      RuleContext ruleContext,
-      AnalysisTestResultInfo infoObject) {
-    FileWriteAction action;
+  public static void writeAnalysisTestAction(
+      RuleContext ruleContext, AnalysisTestResultInfo testResultInfo) {
     // TODO(laszlocsomor): Use the execution platform, not the host platform.
     boolean isExecutedOnWindows = OS.getCurrent() == OS.WINDOWS;
-
+    String escapedMessage =
+        isExecutedOnWindows
+            ? testResultInfo.getMessage().replace("%", "%%")
+            // Prefix each character with \ (double-escaped; once in the string, once in the
+            // replacement sequence, which allows backslash-escaping literal "$"). "." is put in
+            // parentheses because ErrorProne is overly vigorous about objecting to "." as an
+            // always-matching regex (b/201772278).
+            : testResultInfo.getMessage().replaceAll("(.)", "\\\\$1");
+    StringBuilder sb = new StringBuilder();
     if (isExecutedOnWindows) {
-      StringBuilder sb = new StringBuilder().append("@echo off\n");
-      for (String line : Splitter.on("\n").split(infoObject.getMessage())) {
-        sb.append("echo ").append(line).append("\n");
-      }
-      String content = sb
-          .append("exit /b ").append(infoObject.getSuccess() ? "0" : "1")
-          .toString();
-
-      action = FileWriteAction.create(ruleContext,
-          ruleContext.createOutputArtifactScript(), content, /* executable */ true);
-
-    } else {
-      String content =
-          "cat << EOF\n"
-              + infoObject.getMessage()
-              + "\n"
-              + "EOF\n"
-              + "exit "
-              + (infoObject.getSuccess() ? "0" : "1");
-      action = FileWriteAction.create(ruleContext,
-          ruleContext.createOutputArtifactScript(), content, /* executable */ true);
+      sb.append("@echo off\n");
     }
-
+    for (String line : Splitter.on("\n").split(escapedMessage)) {
+      sb.append("echo ").append(line).append("\n");
+    }
+    sb.append("exit ");
+    if (isExecutedOnWindows) {
+      sb.append("/b ");
+    }
+    sb.append(testResultInfo.getSuccess() ? "0" : "1");
+    FileWriteAction action =
+        FileWriteAction.create(
+            ruleContext,
+            ruleContext.createOutputArtifactScript(),
+            sb.toString(),
+            /*makeExecutable=*/ true);
     ruleContext.registerAction(action);
-    return action;
   }
 }

src/test/shell/integration/analysis_test_test.sh

@@ -113,6 +113,96 @@ EOF
   expect_log "A failure message!"
 }
 
+function test_failing_test_shell_escape_in_message() {
+  mkdir -p package
+  cat > package/test.bzl <<'EOF'
+def _rule_test_impl(ctx):
+  return [AnalysisTestResultInfo(success = False,
+                                 message = 'Command is not "$1 copy $2"')]
+
+my_rule_test = rule(
+  implementation = _rule_test_impl,
+  analysis_test = True,
+)
+EOF
+
+  cat > package/BUILD <<EOF
+load(":test.bzl", "my_rule_test")
+
+my_rule_test(name = "r")
+EOF
+
+  ! bazel test package:r >& "$TEST_log" || fail "Unexpected success"
+
+  expect_log "FAILED"
+
+  cat "${PRODUCT_NAME}-testlogs/package/r/test.log" > "$TEST_log"
+
+  expect_log 'Command is not "$1 copy $2"'
+}
+
+function test_failing_test_cmd_escape_in_message() {
+  mkdir -p package
+  cat > package/test.bzl <<'EOF'
+def _rule_test_impl(ctx):
+  return [
+      AnalysisTestResultInfo(
+          success = False,
+          message = 'Command should contain "\\ & < > | ^ ! %FOO%"',
+      ),
+  ]
+
+my_rule_test = rule(
+  implementation = _rule_test_impl,
+  analysis_test = True,
+)
+EOF
+
+  cat > package/BUILD <<EOF
+load(":test.bzl", "my_rule_test")
+
+my_rule_test(name = "r")
+EOF
+
+  ! bazel test package:r >& "$TEST_log" || fail "Unexpected success"
+
+  expect_log "FAILED"
+
+  cat "${PRODUCT_NAME}-testlogs/package/r/test.log" > "$TEST_log"
+
+  expect_log 'Command should contain "\\ & < > \| ^ ! %FOO%"'
+}
+
+function test_failing_test_eof_string_in_message() {
+  mkdir -p package
+  cat > package/test.bzl <<'EOF'
+def _rule_test_impl(ctx):
+  return [AnalysisTestResultInfo(success = False,
+                                 message = '"\nEOF\n" not in command')]
+
+my_rule_test = rule(
+  implementation = _rule_test_impl,
+  analysis_test = True,
+)
+EOF
+
+  cat > package/BUILD <<EOF
+load(":test.bzl", "my_rule_test")
+
+my_rule_test(name = "r")
+EOF
+
+  ! bazel test package:r >& "$TEST_log" || fail "Unexpected success"
+
+  expect_log "FAILED"
+
+  cat "${PRODUCT_NAME}-testlogs/package/r/test.log" > "$TEST_log"
+
+  # expect_log uses grep and looks at individual lines, but we can make sure
+  # the part after \nEOF\n isn't cut off, as it was previously.
+  expect_log "\" not in command"
+}
+
 function test_expected_failure_test() {
   mkdir -p package
   cat > package/test.bzl <<EOF