Skip to content

Commit

Permalink
Ensure non-ambiguous args/env vars injection into PEXes (pantsbuild#1…
Browse files Browse the repository at this point in the history
…8861)

This fixes pantsbuild#18779 by ensuring that injecting arguments and environment
variables into a pex doesn't risk them being interpreted by the pex CLI
itself if they start with `-`, and ensuring they're quoted appropriately
to match the `shlex.split` within pex
(https://github.com/pantsbuild/pex/blob/ff220b9e41484450e0c78136f98d6899f2e2325c/pex/bin/pex.py#L433).

The issue noted in pantsbuild#18779 was specifically ambiguity with `args`, but
the same problem will apply to `env` and so that is fixed here too
(although it's rather less common to have env vars starting with `-`).
NB. `shlex.quote`ing alone won't handle the `-` ambiguity,
`shlex.quote("--foo") == "--foo"`.

For a target like `pex_binary(..., args=["--foo"], env={"--bar":
"baz"})`:

- Previously, this would invoke `pex ... --inject-args --foo
--inject-env --bar=baz`, and argparse within the pex CLI will intepret
`--foo` and `--bar=...` as independent parameters, rather than arguments
to `--inject-args`/`--inject-env`.
- After this change (as suggested by @jsirois), the invocation will be
`pex ... --inject-args=--foo --inject-env=--bar=baz` and there's no risk
of misinterpretation.

For a target like `pex_binary(..., args=["spaces 'n' quotes"])`:

- Previously this would invoke with (equivalently) an arg
`--inject-args=spaces 'n' quotes`, which would `shlex.split` to three
arguments in the PEX, the same as passing `args=["spaces", "n",
"quotes"])`
- Now, this will invoke with `--inject-args='spaces '"'"'n'"'"'
quotes'`, which, is correctly `shlex.split` to the single value: `spaces
'n' quotes`.
  • Loading branch information
huonw committed May 1, 2023
1 parent c539080 commit 1b313a5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,8 @@ def test_layout(rule_runner: RuleRunner, layout: PexLayout) -> None:
"""\
import os
import sys
print(f"FOO={os.environ.get('FOO')}")
print(f"BAR={os.environ.get('BAR')}")
for env in ["FOO", "--inject-arg", "quotes '"]:
print(f"{env}={os.environ.get(env)}")
print(f"ARGV={sys.argv[1:]}")
"""
),
Expand All @@ -187,8 +187,8 @@ def test_layout(rule_runner: RuleRunner, layout: PexLayout) -> None:
python_sources(name="lib")
pex_binary(
entry_point="app.py",
args=['123', 'abc'],
env={{'FOO': 'xxx', 'BAR': 'yyy'}},
args=['123', 'abc', '--inject-env', "quotes 'n spaces"],
env={{'FOO': 'xxx', '--inject-arg': 'yyy', "quotes '": 'n spaces'}},
layout="{layout.value}",
)
"""
Expand All @@ -212,8 +212,9 @@ def test_layout(rule_runner: RuleRunner, layout: PexLayout) -> None:
stdout = dedent(
"""\
FOO=xxx
BAR=yyy
ARGV=['123', 'abc']
--inject-arg=yyy
quotes '=n spaces
ARGV=['123', 'abc', '--inject-env', "quotes 'n spaces"]
"""
).encode()
assert stdout == subprocess.run([executable], check=True, stdout=subprocess.PIPE).stdout
Expand Down
8 changes: 4 additions & 4 deletions src/python/pants/backend/python/util_rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -615,10 +615,10 @@ async def build_pex(
if request.main is not None:
argv.extend(request.main.iter_pex_args())

for injected_arg in request.inject_args:
argv.extend(["--inject-args", str(injected_arg)])
for k, v in sorted(request.inject_env.items()):
argv.extend(["--inject-env", f"{k}={v}"])
argv.extend(
f"--inject-args={shlex.quote(injected_arg)}" for injected_arg in request.inject_args
)
argv.extend(f"--inject-env={k}={v}" for k, v in sorted(request.inject_env.items()))

# TODO(John Sirois): Right now any request requirements will shadow corresponding pex path
# requirements, which could lead to problems. Support shading python binaries.
Expand Down

0 comments on commit 1b313a5

Please sign in to comment.