Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure non-ambiguous args/env vars injection into PEXes (Cherry-pick of #18861) #18877

Merged
merged 1 commit into from
May 2, 2023

Conversation

huonw
Copy link
Contributor

@huonw huonw commented May 1, 2023

This fixes #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 #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.quoteing 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.

…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`.
@huonw huonw added the category:bugfix Bug fixes for released features label May 1, 2023
@huonw huonw requested review from jsirois and kaos May 1, 2023 22:12
@huonw huonw merged commit f02bf4a into pantsbuild:2.16.x May 2, 2023
@huonw huonw deleted the cherry-pick-18861-to-2.16.x branch May 2, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants