From 1cf17b187e7b902dc4112d72a18259b7e574f798 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Mon, 8 Apr 2019 17:46:17 +0200 Subject: [PATCH] Windows, python: add inc. flag to fix esacping See https://github.com/bazelbuild/bazel/issues/7958 RELNOTES[NEW]: Windows, Python: the --incompatible_windows_escape_python_args flag (false by default) builds py_binary and py_test targets with correct command line argument escaping. Change-Id: I789f9370e2cf59fa1a179716ca1c6ad80e1d583e --- .../rules/python/BazelPythonSemantics.java | 22 +- .../lib/rules/python/PythonConfiguration.java | 10 +- .../python/PythonConfigurationLoader.java | 3 +- .../build/lib/rules/python/PythonOptions.java | 20 ++ src/test/shell/integration/BUILD | 7 + .../integration/jvm_flags_escaping_test.sh | 4 +- .../integration/py_args_escaping_test.sh | 300 ++++++++++++++++++ src/tools/launcher/python_launcher.cc | 9 +- 8 files changed, 365 insertions(+), 10 deletions(-) create mode 100755 src/test/shell/integration/py_args_escaping_test.sh diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 10906a5a3c28f9..b09f7d0710a8be 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -145,8 +145,15 @@ public Artifact createExecutable( if (OS.getCurrent() == OS.WINDOWS) { // On Windows, use a Windows native binary to launch the python launcher script (stub file). stubOutput = common.getPythonLauncherArtifact(executable); + + boolean windowsEscapePythonArgs = + ruleContext + .getConfiguration() + .getFragment(PythonConfiguration.class) + .windowsEscapePythonArgs(); executable = - createWindowsExeLauncher(ruleContext, pythonBinary, executable, /*useZipFile*/ false); + createWindowsExeLauncher(ruleContext, pythonBinary, executable, /*useZipFile*/ false, + windowsEscapePythonArgs); } ruleContext.registerAction( @@ -198,7 +205,14 @@ public Artifact createExecutable( .setMnemonic("BuildBinary") .build(ruleContext)); } else { - return createWindowsExeLauncher(ruleContext, pythonBinary, executable, true); + boolean windowsEscapePythonArgs = + ruleContext + .getConfiguration() + .getFragment(PythonConfiguration.class) + .windowsEscapePythonArgs(); + + return createWindowsExeLauncher(ruleContext, pythonBinary, executable, true, + windowsEscapePythonArgs); } } @@ -206,7 +220,8 @@ public Artifact createExecutable( } private static Artifact createWindowsExeLauncher( - RuleContext ruleContext, String pythonBinary, Artifact pythonLauncher, boolean useZipFile) + RuleContext ruleContext, String pythonBinary, Artifact pythonLauncher, boolean useZipFile, + boolean windowsEscapePythonArgs) throws InterruptedException { LaunchInfo launchInfo = LaunchInfo.builder() @@ -217,6 +232,7 @@ private static Artifact createWindowsExeLauncher( ruleContext.getConfiguration().runfilesEnabled() ? "1" : "0") .addKeyValuePair("python_bin_path", pythonBinary) .addKeyValuePair("use_zip_file", useZipFile ? "1" : "0") + .addKeyValuePair("escape_args", windowsEscapePythonArgs ? "1" : "0") .build(); LauncherFileWriteAction.createAndRegister(ruleContext, pythonLauncher, launchInfo); return pythonLauncher; diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java index a5e863f7813b9c..7c2291d3ea4071 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfiguration.java @@ -55,6 +55,8 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { // TODO(brandjon): Remove this once migration to Python toolchains is complete. private final boolean useToolchains; + private final boolean windowsEscapePythonArgs; + PythonConfiguration( PythonVersion version, PythonVersion defaultVersion, @@ -64,7 +66,8 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { boolean useNewPyVersionSemantics, boolean py2OutputsAreSuffixed, boolean disallowLegacyPyProvider, - boolean useToolchains) { + boolean useToolchains, + boolean windowsEscapePythonArgs) { this.version = version; this.defaultVersion = defaultVersion; this.buildPythonZip = buildPythonZip; @@ -74,6 +77,7 @@ public class PythonConfiguration extends BuildConfiguration.Fragment { this.py2OutputsAreSuffixed = py2OutputsAreSuffixed; this.disallowLegacyPyProvider = disallowLegacyPyProvider; this.useToolchains = useToolchains; + this.windowsEscapePythonArgs = windowsEscapePythonArgs; } /** @@ -188,4 +192,8 @@ public boolean disallowLegacyPyProvider() { public boolean useToolchains() { return useToolchains; } + + public boolean windowsEscapePythonArgs() { + return windowsEscapePythonArgs; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java index e8d8b70295f658..8b74f2a1d4e37c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonConfigurationLoader.java @@ -43,7 +43,8 @@ public PythonConfiguration create(BuildOptions buildOptions) /*useNewPyVersionSemantics=*/ pythonOptions.incompatibleAllowPythonVersionTransitions, /*py2OutputsAreSuffixed=*/ pythonOptions.incompatiblePy2OutputsAreSuffixed, /*disallowLegacyPyProvider=*/ pythonOptions.incompatibleDisallowLegacyPyProvider, - /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains); + /*useToolchains=*/ pythonOptions.incompatibleUsePythonToolchains, + pythonOptions.windowsEscapePythonArgs); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java index f958ce00e7fd3d..e14b36f76af2bb 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/python/PythonOptions.java @@ -254,6 +254,26 @@ public String getTypeDescription() { + "data runfiles of another binary.") public boolean buildTransitiveRunfilesTrees; + @Option( + name = "incompatible_windows_escape_python_args", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS, + effectTags = { + OptionEffectTag.ACTION_COMMAND_LINES, + OptionEffectTag.AFFECTS_OUTPUTS, + }, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "On Linux/macOS/non-Windows: no-op. On Windows: this flag affects how py_binary and" + + " py_test targets are built: how their launcher escapes command line flags. When" + + " this flag is true, the launcher escapes command line flags using Windows-style" + + " escaping (correct behavior). When the flag is false, the launcher uses Bash-style" + + " escaping (buggy behavior). See https://github.com/bazelbuild/bazel/issues/7958") + public boolean windowsEscapePythonArgs; + @Override public Map getSelectRestrictions() { // TODO(brandjon): Instead of referencing the python_version target, whose path depends on the diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD index 2e4ae0dcb34c90..b5233b0b364d0e 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -600,6 +600,13 @@ sh_test( deps = ["@bazel_tools//tools/bash/runfiles"], ) +sh_test( + name = "py_args_escaping_test", + srcs = ["py_args_escaping_test.sh"], + data = [":test-deps"], + deps = ["@bazel_tools//tools/bash/runfiles"], +) + ######################################################################## # Test suites. diff --git a/src/test/shell/integration/jvm_flags_escaping_test.sh b/src/test/shell/integration/jvm_flags_escaping_test.sh index 9d8041204122da..2cf1205deb73e3 100755 --- a/src/test/shell/integration/jvm_flags_escaping_test.sh +++ b/src/test/shell/integration/jvm_flags_escaping_test.sh @@ -318,9 +318,7 @@ function test_untokenizable_jvm_flag_when_escaping_is_enabled() { local -r flag="--incompatible_windows_escape_jvm_flags" if "$is_windows"; then - # On Windows, Bazel will check the flag. It will just propagate the flag - # to the launcher, which also just passes the flag to the JVM. This is bad, - # the flag should have been rejected because it cannot be Bash-tokenized. + # On Windows, Bazel will check the flag. bazel build --verbose_failures "$flag" "${pkg}:cannot_tokenize" \ 2>"$TEST_log" && fail "expected failure" || true expect_log "ERROR:.*in jvm_flags attribute of java_binary rule" diff --git a/src/test/shell/integration/py_args_escaping_test.sh b/src/test/shell/integration/py_args_escaping_test.sh new file mode 100755 index 00000000000000..0ff1495a7dad3f --- /dev/null +++ b/src/test/shell/integration/py_args_escaping_test.sh @@ -0,0 +1,300 @@ +#!/bin/bash +# +# Copyright 2019 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# --- begin runfiles.bash initialization --- +# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash). +set -euo pipefail +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ + || { echo "integration_test_setup.sh not found!" >&2; exit 1; } + +# `uname` returns the current platform, e.g "MSYS_NT-10.0" or "Linux". +# `tr` converts all upper case letters to lower case. +# `case` matches the result if the `uname | tr` expression to string prefixes +# that use the same wildcards as names do in Bash, i.e. "msys*" matches strings +# starting with "msys", and "*" matches everything (it's the default case). +case "$(uname -s | tr [:upper:] [:lower:])" in +msys*) + # As of 2019-04-08, Bazel on Windows only supports MSYS Bash. + declare -r is_windows=true + ;; +*) + declare -r is_windows=false + ;; +esac + +if "$is_windows"; then + # Disable MSYS path conversion that converts path-looking command arguments to + # Windows paths (even if they arguments are not in fact paths). + export MSYS_NO_PATHCONV=1 + export MSYS2_ARG_CONV_EXCL="*" + declare -r EXE_EXT=".exe" +else + declare -r EXE_EXT="" +fi + +# ---------------------------------------------------------------------- +# HELPER FUNCTIONS +# ---------------------------------------------------------------------- + +# Writes a python file that prints all arguments (except argv[0]). +# +# Args: +# $1: directory (package path) where the file will be written +function create_py_file_that_prints_args() { + local -r pkg="$1"; shift + mkdir -p "$pkg" || fail "mkdir -p $pkg" + cat >"$pkg/a.py" <<'eof' +from __future__ import print_function +import sys +for i in range(1, len(sys.argv)): + print("arg%d=(%s)" % (i, sys.argv[i])) +eof +} + +# Writes a BUILD file for a py_binary with an untokenizable "args" entry. +# +# Args: +# $1: directory (package path) where the file will be written +function create_build_file_for_untokenizable_args() { + local -r pkg="$1"; shift + mkdir -p "$pkg" || fail "mkdir -p $pkg" + cat >"$pkg/BUILD" <<'eof' +py_binary( + name = "cannot_tokenize", + srcs = ["a.py"], + main = "a.py", + args = ["'abc"], +) +eof +} + +# Writes a BUILD file for a py_binary with many different "args" entries. +# +# Use this together with assert_*_output_of_the_program_with_many_args(). +# +# Args: +# $1: directory (package path) where the file will be written +function create_build_file_with_many_args() { + local -r pkg="$1"; shift + mkdir -p "$pkg" || fail "mkdir -p $pkg" + cat >"$pkg/BUILD" <<'eof' +py_binary( + name = "x", + srcs = ["a.py"], + main = "a.py", + args = [ + "''", + "' '", + "'\"'", + "'\"\\'", + "'\\'", + "'\\\"'", + "'with space'", + "'with^caret'", + "'space ^caret'", + "'caret^ space'", + "'with\"quote'", + "'with\\backslash'", + "'one\\ backslash and \\space'", + "'two\\\\backslashes'", + "'two\\\\ backslashes \\\\and space'", + "'one\\\"x'", + "'two\\\\\"x'", + "'a \\ b'", + "'a \\\" b'", + "'A'", + "'\"a\"'", + "'B C'", + "'\"b c\"'", + "'D\"E'", + "'\"d\"e\"'", + "'C:\\F G'", + "'\"C:\\f g\"'", + "'C:\\H\"I'", + "'\"C:\\h\"i\"'", + "'C:\\J\\\"K'", + "'\"C:\\j\\\"k\"'", + "'C:\\L M '", + "'\"C:\\l m \"'", + "'C:\\N O\\'", + "'\"C:\\n o\\\"'", + "'C:\\P Q\\ '", + "'\"C:\\p q\\ \"'", + "'C:\\R\\S\\'", + "'C:\\R x\\S\\'", + "'\"C:\\r\\s\\\"'", + "'\"C:\\r x\\s\\\"'", + "'C:\\T U\\W\\'", + "'\"C:\\t u\\w\\\"'", + "a", + r"b\\\"", + "c", + ], +) +eof +} + +# Asserts that the $TEST_log contains bad py_binary args. +# +# This assertion guards (and demonstrates) the status quo. +# +# See create_build_file_with_many_args() and create_py_file_that_prints_args(). +function assert_bad_output_of_the_program_with_many_args() { + expect_log 'arg1=()' + expect_log 'arg2=( )' + expect_log 'arg3=(")' + expect_log 'arg4=("\\)' + # arg5 and arg6 already stumble. No need to assert more. + expect_log 'arg5=(\\)' + expect_log 'arg6=(\\ with)' + # To illustrate the bug again, these args match those in the bug report: + # https://github.com/bazelbuild/bazel/issues/7958 + expect_log 'arg40=(a)' + expect_log 'arg41=(b\\ c)' +} + +# Asserts that the $TEST_log contains all py_binary args as argN=(VALUE). +# +# See create_build_file_with_many_args() and create_py_file_that_prints_args(). +function assert_good_output_of_the_program_with_many_args() { + expect_log 'arg1=()' + expect_log 'arg2=( )' + expect_log 'arg3=(")' + expect_log 'arg4=("\\)' + expect_log 'arg5=(\\)' + expect_log 'arg6=(\\")' + expect_log 'arg7=(with space)' + expect_log 'arg8=(with^caret)' + expect_log 'arg9=(space ^caret)' + expect_log 'arg10=(caret^ space)' + expect_log 'arg11=(with"quote)' + expect_log 'arg12=(with\\backslash)' + expect_log 'arg13=(one\\ backslash and \\space)' + expect_log 'arg14=(two\\\\backslashes)' + expect_log 'arg15=(two\\\\ backslashes \\\\and space)' + expect_log 'arg16=(one\\"x)' + expect_log 'arg17=(two\\\\"x)' + expect_log 'arg18=(a \\ b)' + expect_log 'arg19=(a \\" b)' + expect_log 'arg20=(A)' + expect_log 'arg21=("a")' + expect_log 'arg22=(B C)' + expect_log 'arg23=("b c")' + expect_log 'arg24=(D"E)' + expect_log 'arg25=("d"e")' + expect_log 'arg26=(C:\\F G)' + expect_log 'arg27=("C:\\f g")' + expect_log 'arg28=(C:\\H"I)' + expect_log 'arg29=("C:\\h"i")' + expect_log 'arg30=(C:\\J\\"K)' + expect_log 'arg31=("C:\\j\\"k")' + expect_log 'arg32=(C:\\L M )' + expect_log 'arg33=("C:\\l m ")' + expect_log 'arg34=(C:\\N O\\)' + expect_log 'arg35=("C:\\n o\\")' + expect_log 'arg36=(C:\\P Q\\ )' + expect_log 'arg37=("C:\\p q\\ ")' + expect_log 'arg38=(C:\\R\\S\\)' + expect_log 'arg39=(C:\\R x\\S\\)' + expect_log 'arg40=("C:\\r\\s\\")' + expect_log 'arg41=("C:\\r x\\s\\")' + expect_log 'arg42=(C:\\T U\\W\\)' + expect_log 'arg43=("C:\\t u\\w\\")' + expect_log 'arg44=(a)' + expect_log 'arg45=(b\\")' + expect_log 'arg46=(c)' +} + +# ---------------------------------------------------------------------- +# TESTS +# ---------------------------------------------------------------------- + +function test_args_escaping_disabled_on_windows() { + local -r pkg="${FUNCNAME[0]}" # unique package name for this test + + create_py_file_that_prints_args "$pkg" + create_build_file_with_many_args "$pkg" + + bazel run --verbose_failures --noincompatible_windows_escape_python_args \ + "${pkg}:x" &>"$TEST_log" || fail "expected success" + if "$is_windows"; then + # On Windows, the target runs but prints bad output. + assert_bad_output_of_the_program_with_many_args + else + # On other platforms, the program runs fine and prints correct output. + assert_good_output_of_the_program_with_many_args + fi +} + +function test_args_escaping() { + local -r pkg="${FUNCNAME[0]}" # unique package name for this test + + create_py_file_that_prints_args "$pkg" + create_build_file_with_many_args "$pkg" + + # On all platforms, the target prints good output. + bazel run --verbose_failures --incompatible_windows_escape_python_args \ + "${pkg}:x" &>"$TEST_log" || fail "expected success" + assert_good_output_of_the_program_with_many_args +} + +function test_untokenizable_args_when_escaping_is_disabled() { + local -r pkg="${FUNCNAME[0]}" # unique package name for this test + + create_py_file_that_prints_args "$pkg" + create_build_file_for_untokenizable_args "$pkg" + + # On all platforms, Bazel can build the target. + if bazel build --verbose_failures --noincompatible_windows_escape_python_args \ + "${pkg}:cannot_tokenize" 2>"$TEST_log"; then + fail "expected failure" + fi + expect_log "unterminated quotation" +} + +function test_untokenizable_args_when_escaping_is_enabled() { + local -r pkg="${FUNCNAME[0]}" # unique package name for this test + + create_py_file_that_prints_args "$pkg" + create_build_file_for_untokenizable_args "$pkg" + + local -r flag="--incompatible_windows_escape_python_args" + bazel run --verbose_failures "$flag" "${pkg}:cannot_tokenize" \ + 2>"$TEST_log" && fail "expected failure" || true + expect_log "ERROR:.*in args attribute of py_binary rule.*unterminated quotation" +} + +run_suite "Tests about how Bazel passes py_binary.args to the binary" diff --git a/src/tools/launcher/python_launcher.cc b/src/tools/launcher/python_launcher.cc index 5332ab1eeb801b..c4d2d5c04670fd 100644 --- a/src/tools/launcher/python_launcher.cc +++ b/src/tools/launcher/python_launcher.cc @@ -26,6 +26,7 @@ using std::wstring; static constexpr const char* PYTHON_BIN_PATH = "python_bin_path"; static constexpr const char* USE_ZIP_FILE = "use_zip_file"; +static constexpr const char* WINDOWS_STYLE_ESCAPE_JVM_FLAGS = "escape_args"; ExitCode PythonBinaryLauncher::Launch() { wstring python_binary = this->GetLaunchInfoByKey(PYTHON_BIN_PATH); @@ -47,9 +48,13 @@ ExitCode PythonBinaryLauncher::Launch() { // Replace the first argument with python file path args[0] = python_file; - // Escape arguments that has spaces + wstring (*const escape_arg_func)(const wstring&) = + this->GetLaunchInfoByKey(WINDOWS_STYLE_ESCAPE_JVM_FLAGS) == L"1" + ? WindowsEscapeArg2 + : WindowsEscapeArg; + for (int i = 1; i < args.size(); i++) { - args[i] = WindowsEscapeArg(args[i]); + args[i] = escape_arg_func(args[i]); } return this->LaunchProcess(python_binary, args);