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

podman generate systemd --new can be broken by environment variable substitution and specifier expansion #9176

Closed
ntkme opened this issue Feb 1, 2021 · 5 comments · Fixed by #9178
Assignees
Labels
In Progress This issue is actively being worked by the assignee, please do not work on this at this time. kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@ntkme
Copy link
Contributor

ntkme commented Feb 1, 2021

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

When you have a container with environment variable syntax in command or systemd specifiers, systemd will attempt to expand those result in broken container.

Steps to reproduce the issue:

  1. podman run -d --name sleep alpine sh -c 'kill -0 $$ && sleep infinity'
  2. podman generate systemd --name --new sleep
  3. Start the generated service.

Describe the results you received:

Container will crash with error kill: illegal pid: $. The reason is that systemd will expand environment variable before executing the command. Also, systemd uses $$ to escape a single $, therefore the executed command will be no longer the same as the original command.

Describe the results you expected:

Container will start in the same way as original podman command.

Additional information you deem important (e.g. issue happens only occasionally):

If the executable path is prefixed with ":", environment variable substitution (as described by the "Command Lines" section below) is not applied.

However, this issue is not only limited to literal $ in command line being expanded environment variable as by systemd, literal % may also expanded by systemd as specifiers. For example, if I have printf '%s' 'test', it will become printf '/bin/sh' 'test'.

Reference for how systemd expand command line:
https://www.freedesktop.org/software/systemd/man/systemd.service.html#ExecStart=
https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines
https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Specifiers

Output of podman version:

Version:      2.2.1
API Version:  2.1.0
Go Version:   go1.15.5
Built:        Tue Dec  8 14:38:08 2020
OS/Arch:      linux/arm64
@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 1, 2021
@ntkme ntkme changed the title podman generate systemd can be broken by environment variable substitution podman generate systemd can be broken by environment variable substitution and specifier expansion Feb 1, 2021
@ntkme ntkme changed the title podman generate systemd can be broken by environment variable substitution and specifier expansion podman generate systemd --new can be broken by environment variable substitution and specifier expansion Feb 1, 2021
@ntkme
Copy link
Contributor Author

ntkme commented Feb 1, 2021

Not sure if it is sufficient, but I think replacing $ with $$ and % with %% for the command line arguments given to the container should fix this.

@vrothberg
Copy link
Member

Thanks for reaching out, @ntkme!

Not sure if it is sufficient, but I think replacing $ with $$ and % with %% for the command line arguments given to the container should fix this.

I am open to this idea. @Luap99 what do you think?

@Luap99
Copy link
Member

Luap99 commented Feb 1, 2021

SGTM, we also have to escape a backslash according to the systemd documentation.

@vrothberg
Copy link
Member

SGTM, we also have to escape a backslash according to the systemd documentation.

Perfect! Do you have cycles to tackle the issue? I appreciated to have another pair of eyes on the systemd-gen code :)

@Luap99
Copy link
Member

Luap99 commented Feb 1, 2021

Yes I will take this one.

@Luap99 Luap99 self-assigned this Feb 1, 2021
@Luap99 Luap99 added the In Progress This issue is actively being worked by the assignee, please do not work on this at this time. label Feb 1, 2021
Luap99 pushed a commit to Luap99/libpod that referenced this issue Feb 1, 2021
In a systemd unit dollar and percent signs are used for variables. A backslash
is used for escape sequences. If any of these characters are used in the create
command we have to properly escape them so systemd does not try to interpret them.

Fixes containers#9176

Signed-off-by: Paul Holzinger <[email protected]>
mheon pushed a commit to mheon/libpod that referenced this issue Feb 4, 2021
In a systemd unit dollar and percent signs are used for variables. A backslash
is used for escape sequences. If any of these characters are used in the create
command we have to properly escape them so systemd does not try to interpret them.

Fixes containers#9176

Signed-off-by: Paul Holzinger <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
In Progress This issue is actively being worked by the assignee, please do not work on this at this time. kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants