From 8d1176110704a427644d0a09fb02789d597613ce Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 12:20:00 +0000 Subject: [PATCH 1/7] Use the effective runtime to determine whether `-P` can be added to the interpreter options --- python/private/common/py_executable_bazel.bzl | 16 +++++++++- python/private/python_bootstrap_template.txt | 30 +++++++++---------- python/private/stage1_bootstrap_template.sh | 18 ++++------- python/private/zip_main_template.py | 9 ++---- 4 files changed, 39 insertions(+), 34 deletions(-) diff --git a/python/private/common/py_executable_bazel.bzl b/python/private/common/py_executable_bazel.bzl index a0cfebad8a..564c62a5af 100644 --- a/python/private/common/py_executable_bazel.bzl +++ b/python/private/common/py_executable_bazel.bzl @@ -168,6 +168,14 @@ def _create_executable( runfiles_details): _ = is_test, cc_details, native_deps_details # @unused + # If the runtime interpreter has been detected and its version is 3.11 or higher, enable safe-path mode + # by passing the -P argument to the interpreter + interpreter_version_info = runtime_details.effective_runtime.interpreter_version_info + if interpreter_version_info and interpreter_version_info.major >= 3 and interpreter_version_info.minor >= 11: + interpreter_opts = "-P" + else: + interpreter_opts = "" + is_windows = target_platform_has_any_constraint(ctx, ctx.attr._windows_constraints) if is_windows: @@ -207,6 +215,7 @@ def _create_executable( output = zip_main, main_py = main_py, imports = imports, + interpreter_opts = interpreter_opts, is_for_zip = True, runtime_details = runtime_details, ) @@ -270,6 +279,7 @@ def _create_executable( ctx, output = executable, zip_file = zip_file, + interpreter_opts = interpreter_opts, stage2_bootstrap = stage2_bootstrap, runtime_details = runtime_details, ) @@ -281,6 +291,7 @@ def _create_executable( runtime_details = runtime_details, is_for_zip = False, imports = imports, + interpreter_opts = interpreter_opts, main_py = main_py, ) else: @@ -368,11 +379,13 @@ def _create_stage1_bootstrap( main_py = None, stage2_bootstrap = None, imports = None, + interpreter_opts, is_for_zip, runtime_details): runtime = runtime_details.effective_runtime subs = { + "%interpreter_opts%": interpreter_opts, "%is_zipfile%": "1" if is_for_zip else "0", "%python_binary%": runtime_details.executable_interpreter_path, "%target%": str(ctx.label), @@ -522,7 +535,7 @@ def _get_zip_runfiles_path(path, workspace_name, legacy_external_runfiles): zip_runfiles_path = paths.normalize("{}/{}".format(workspace_name, path)) return "{}/{}".format(_ZIP_RUNFILES_DIRECTORY_NAME, zip_runfiles_path) -def _create_executable_zip_file(ctx, *, output, zip_file, stage2_bootstrap, runtime_details): +def _create_executable_zip_file(ctx, *, output, zip_file, interpreter_opts, stage2_bootstrap, runtime_details): prelude = ctx.actions.declare_file( "{}_zip_prelude.sh".format(output.basename), sibling = output, @@ -533,6 +546,7 @@ def _create_executable_zip_file(ctx, *, output, zip_file, stage2_bootstrap, runt output = prelude, stage2_bootstrap = stage2_bootstrap, runtime_details = runtime_details, + interpreter_opts = interpreter_opts, is_for_zip = True, ) else: diff --git a/python/private/python_bootstrap_template.txt b/python/private/python_bootstrap_template.txt index 0f9c90b3b3..9e45d040b6 100644 --- a/python/private/python_bootstrap_template.txt +++ b/python/private/python_bootstrap_template.txt @@ -32,6 +32,9 @@ if IsRunningFromZip(): else: import re +def GetInterpreterOpts(): + return "%interpreter_opts%".split() + # Return True if running on Windows def IsWindows(): return os.name == 'nt' @@ -363,8 +366,9 @@ def ExecuteFile(python_program, main_filename, args, env, module_space, ret_code = _RunForCoverage(python_program, main_filename, args, env, coverage_entrypoint, workspace) else: + subprocess_argv = [python_program] + GetInterpreterOpts() + [main_filename] + args ret_code = subprocess.call( - [python_program, main_filename] + args, + subprocess_argv, env=env, cwd=workspace ) @@ -381,7 +385,7 @@ def _RunExecv(python_program, main_filename, args, env): """Executes the given Python file using the various environment settings.""" os.environ.update(env) PrintVerbose("RunExecv: environ:", os.environ) - argv = [python_program, main_filename] + args + argv = [python_program] + GetInterpreterOpts() + [main_filename] + args PrintVerbose("RunExecv: argv:", python_program, argv) os.execv(python_program, argv) @@ -410,16 +414,16 @@ relative_files = True PrintVerboseCoverage('Coverage entrypoint:', coverage_entrypoint) # First run the target Python file via coveragepy to create a .coverage # database file, from which we can later export lcov. + subprocess_argv = [python_program] + GetInterpreterOpts() + [ + coverage_entrypoint, + "run", + "--rcfile=" + rcfile_name, + "--append", + "--branch", + main_filename + ] + args ret_code = subprocess.call( - [ - python_program, - coverage_entrypoint, - "run", - "--rcfile=" + rcfile_name, - "--append", - "--branch", - main_filename - ] + args, + subprocess_argv, env=env, cwd=workspace ) @@ -495,10 +499,6 @@ def Main(): if runfiles_envkey: new_env[runfiles_envkey] = runfiles_envvalue - # Don't prepend a potentially unsafe path to sys.path - # See: https://docs.python.org/3.11/using/cmdline.html#envvar-PYTHONSAFEPATH - new_env['PYTHONSAFEPATH'] = '1' - main_filename = os.path.join(module_space, main_rel_path) main_filename = GetWindowsPathWithUNCPrefix(main_filename) assert os.path.exists(main_filename), \ diff --git a/python/private/stage1_bootstrap_template.sh b/python/private/stage1_bootstrap_template.sh index 959e7babe6..a10161da84 100644 --- a/python/private/stage1_bootstrap_template.sh +++ b/python/private/stage1_bootstrap_template.sh @@ -15,6 +15,9 @@ PYTHON_BINARY='%python_binary%' # 0 or 1 IS_ZIPFILE="%is_zipfile%" +# command line options to be passed to the interpreter +INTERPRETER_OPTS="%interpreter_opts%" + if [[ "$IS_ZIPFILE" == "1" ]]; then # NOTE: Macs have an old version of mktemp, so we must use only the # minimal functionality of it. @@ -102,18 +105,9 @@ stage2_bootstrap="$RUNFILES_DIR/$STAGE2_BOOTSTRAP" declare -a interpreter_env declare -a interpreter_args -# Don't prepend a potentially unsafe path to sys.path -# See: https://docs.python.org/3.11/using/cmdline.html#envvar-PYTHONSAFEPATH -# NOTE: Only works for 3.11+ -# We inherit the value from the outer environment in case the user wants to -# opt-out of using PYTHONSAFEPATH. To opt-out, they have to set -# `PYTHONSAFEPATH=` (empty string). This is because Python treats the empty -# value as false, and any non-empty value as true. -# ${FOO+WORD} expands to empty if $FOO is undefined, and WORD otherwise. -if [[ -z "${PYTHONSAFEPATH+x}" ]]; then - # ${FOO-WORD} expands to WORD if $FOO is undefined, and $FOO otherwise - interpreter_env+=("PYTHONSAFEPATH=${PYTHONSAFEPATH-1}") -fi +# Split the space-separated values in INTERPRETER_OPTS and store them +# inside the array interpreter_args +read -ra interpreter_args <<< "$INTERPRETER_OPTS" if [[ "$IS_ZIPFILE" == "1" ]]; then interpreter_args+=("-XRULES_PYTHON_ZIP_DIR=$zip_dir") diff --git a/python/private/zip_main_template.py b/python/private/zip_main_template.py index 2d3aea7b7b..b5bec46eaa 100644 --- a/python/private/zip_main_template.py +++ b/python/private/zip_main_template.py @@ -27,12 +27,13 @@ _PYTHON_BINARY = "%python_binary%" _WORKSPACE_NAME = "%workspace_name%" +def get_interpreter_opts(): + return "%interpreter_opts%".split() # Return True if running on Windows def is_windows(): return os.name == "nt" - def get_windows_path_with_unc_prefix(path): """Adds UNC prefix after getting a normalized absolute Windows path. @@ -208,7 +209,7 @@ def execute_file( # - When running in a workspace or zip file, we need to clean up the # workspace after the process finishes so control must return here. try: - subprocess_argv = [python_program, main_filename] + args + subprocess_argv = [python_program] + get_interpreter_opts() + [main_filename] + args print_verbose("subprocess argv:", values=subprocess_argv) print_verbose("subprocess env:", mapping=env) print_verbose("subprocess cwd:", workspace) @@ -244,10 +245,6 @@ def main(): new_env["RUNFILES_DIR"] = module_space - # Don't prepend a potentially unsafe path to sys.path - # See: https://docs.python.org/3.11/using/cmdline.html#envvar-PYTHONSAFEPATH - new_env["PYTHONSAFEPATH"] = "1" - main_filename = os.path.join(module_space, main_rel_path) main_filename = get_windows_path_with_unc_prefix(main_filename) assert os.path.exists(main_filename), ( From 2886cccb1bef8b918c0b292188f2fdd3a8f32567 Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 13:04:05 +0000 Subject: [PATCH 2/7] Check if major and minor version are None before comparison --- python/private/common/py_executable_bazel.bzl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/python/private/common/py_executable_bazel.bzl b/python/private/common/py_executable_bazel.bzl index 564c62a5af..28681710bb 100644 --- a/python/private/common/py_executable_bazel.bzl +++ b/python/private/common/py_executable_bazel.bzl @@ -171,7 +171,10 @@ def _create_executable( # If the runtime interpreter has been detected and its version is 3.11 or higher, enable safe-path mode # by passing the -P argument to the interpreter interpreter_version_info = runtime_details.effective_runtime.interpreter_version_info - if interpreter_version_info and interpreter_version_info.major >= 3 and interpreter_version_info.minor >= 11: + if interpreter_version_info.major and \ + interpreter_version_info.major >= 3 and \ + interpreter_version_info.minor and \ + interpreter_version_info.minor >= 11: interpreter_opts = "-P" else: interpreter_opts = "" From 96c73fe99edad7f96f78fe67523863b440e06a40 Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 13:09:56 +0000 Subject: [PATCH 3/7] Check if interpreter_version_info is None before comparison --- python/private/common/py_executable_bazel.bzl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/private/common/py_executable_bazel.bzl b/python/private/common/py_executable_bazel.bzl index 28681710bb..f3b43f6a21 100644 --- a/python/private/common/py_executable_bazel.bzl +++ b/python/private/common/py_executable_bazel.bzl @@ -171,7 +171,8 @@ def _create_executable( # If the runtime interpreter has been detected and its version is 3.11 or higher, enable safe-path mode # by passing the -P argument to the interpreter interpreter_version_info = runtime_details.effective_runtime.interpreter_version_info - if interpreter_version_info.major and \ + if interpreter_version_info and \ + interpreter_version_info.major and \ interpreter_version_info.major >= 3 and \ interpreter_version_info.minor and \ interpreter_version_info.minor >= 11: From eb8870604f9855dc42249c4f831aa80005022bbe Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 13:25:19 +0000 Subject: [PATCH 4/7] Update python safe-path test --- tests/base_rules/BUILD.bazel | 4 ++-- ...ath_env_test.sh => pythonsafepath_test.sh} | 21 ++++--------------- 2 files changed, 6 insertions(+), 19 deletions(-) rename tests/base_rules/{inherit_pythonsafepath_env_test.sh => pythonsafepath_test.sh} (75%) diff --git a/tests/base_rules/BUILD.bazel b/tests/base_rules/BUILD.bazel index cd5771533d..5fed6eb305 100644 --- a/tests/base_rules/BUILD.bazel +++ b/tests/base_rules/BUILD.bazel @@ -72,9 +72,9 @@ py_reconfig_test( ) sh_py_run_test( - name = "inherit_pythonsafepath_env_test", + name = "pythonsafepath_test", bootstrap_impl = "script", py_src = "bin.py", - sh_src = "inherit_pythonsafepath_env_test.sh", + sh_src = "pythonsafepath_test.sh", target_compatible_with = _SUPPORTS_BOOTSTRAP_SCRIPT, ) diff --git a/tests/base_rules/inherit_pythonsafepath_env_test.sh b/tests/base_rules/pythonsafepath_test.sh similarity index 75% rename from tests/base_rules/inherit_pythonsafepath_env_test.sh rename to tests/base_rules/pythonsafepath_test.sh index bc6e2d53f3..642d8ba9e3 100755 --- a/tests/base_rules/inherit_pythonsafepath_env_test.sh +++ b/tests/base_rules/pythonsafepath_test.sh @@ -45,25 +45,12 @@ function expect_match() { fi } - -echo "Check inherited and disabled" -# Verify setting it to empty string disables safe path -actual=$(PYTHONSAFEPATH= $bin 2>&1) -expect_match "sys.flags.safe_path: False" "$actual" -expect_match "PYTHONSAFEPATH: EMPTY" "$actual" - -echo "Check inherited and propagated" -# Verify setting it to any string enables safe path and that -# value is propagated -actual=$(PYTHONSAFEPATH=OUTER $bin 2>&1) -expect_match "sys.flags.safe_path: True" "$actual" -expect_match "PYTHONSAFEPATH: OUTER" "$actual" - -echo "Check enabled by default" -# Verifying doing nothing leaves safepath enabled by default +echo "Check PYTHONSAFEPATH is unset while safe_path is True by default" +# Verify that safe_path is True despite PYTHONSAFEPATH being UNSET (since we +# use the -P argument to set it) actual=$($bin 2>&1) expect_match "sys.flags.safe_path: True" "$actual" -expect_match "PYTHONSAFEPATH: 1" "$actual" +expect_match "PYTHONSAFEPATH: UNSET" "$actual" # Exit if any of the expects failed [[ ! -e EXPECTATION_FAILED ]] From a5d9900bbcb1a6e6851c3ebedb2a9be73a5b2535 Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 13:37:30 +0000 Subject: [PATCH 5/7] Make version checking logic more robust --- python/private/common/py_executable_bazel.bzl | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/python/private/common/py_executable_bazel.bzl b/python/private/common/py_executable_bazel.bzl index f3b43f6a21..df851da595 100644 --- a/python/private/common/py_executable_bazel.bzl +++ b/python/private/common/py_executable_bazel.bzl @@ -168,17 +168,17 @@ def _create_executable( runfiles_details): _ = is_test, cc_details, native_deps_details # @unused + interpreter_opts = "" # If the runtime interpreter has been detected and its version is 3.11 or higher, enable safe-path mode # by passing the -P argument to the interpreter - interpreter_version_info = runtime_details.effective_runtime.interpreter_version_info - if interpreter_version_info and \ - interpreter_version_info.major and \ - interpreter_version_info.major >= 3 and \ - interpreter_version_info.minor and \ - interpreter_version_info.minor >= 11: - interpreter_opts = "-P" - else: - interpreter_opts = "" + if hasattr(runtime_details.effective_runtime, "interpreter_version_info"): + interpreter_version_info = runtime_details.effective_runtime.interpreter_version_info + if interpreter_version_info and \ + interpreter_version_info.major and \ + interpreter_version_info.major >= 3 and \ + interpreter_version_info.minor and \ + interpreter_version_info.minor >= 11: + interpreter_opts += " -P" is_windows = target_platform_has_any_constraint(ctx, ctx.attr._windows_constraints) From ee6049aae1a9ced6cd81ab88288a9113ec5e5431 Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 13:53:47 +0000 Subject: [PATCH 6/7] Add newline to satisify buildifier lints --- python/private/common/py_executable_bazel.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/python/private/common/py_executable_bazel.bzl b/python/private/common/py_executable_bazel.bzl index df851da595..d750189149 100644 --- a/python/private/common/py_executable_bazel.bzl +++ b/python/private/common/py_executable_bazel.bzl @@ -169,6 +169,7 @@ def _create_executable( _ = is_test, cc_details, native_deps_details # @unused interpreter_opts = "" + # If the runtime interpreter has been detected and its version is 3.11 or higher, enable safe-path mode # by passing the -P argument to the interpreter if hasattr(runtime_details.effective_runtime, "interpreter_version_info"): From d674be5434e5a4f42662bcf923b865d748dc849f Mon Sep 17 00:00:00 2001 From: Michael Allwright Date: Mon, 26 Aug 2024 14:00:54 +0000 Subject: [PATCH 7/7] Add entry to CHANGELOG.md --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59dfbe481b..c8a7c928f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,12 @@ A brief description of the categories of changes: ### Changed * (gazelle): Update error messages when unable to resolve a dependency to be more human-friendly. +* (rules) The `PYTHONSAFEPATH` environment variable is no longer used to enable safe-path mode + since it is unintentionally inherited by subprocesses. Instead, `-P` is passed to the runtime + interpreter given its version can be detected and is greater than or equal to 3.11. If the + runtime interpreter can not be detected or the version is less than 3.11, safe-path mode is not + enabled. + ([#2060](https://github.com/bazelbuild/rules_python/issues/2060)). ### Fixed * (whl_library): Remove `--no-index` and add `--no-build-isolation` to the