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.

Sign up for GitHub

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

Jump to bottom

correctly use configured ansible_shell_executable #31361

Merged
merged 1 commit into from
Oct 6, 2017
Merged

correctly use configured ansible_shell_executable #31361

merged 1 commit into from
Oct 6, 2017

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Oct 5, 2017 •
edited

SUMMARY

fixes #30836
fixes #24169 .. correctly this time
fixes #26741 i had misunderstood the issue, now the proper shell should be used, not the default
fixes #30620

ISSUE TYPE

run_command

ANSIBLE VERSION
2.4/2.5

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 bugfix_pull_request module_utils/basic needs_triage Needs a first human triage before being processed. plugins/action support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 5, 2017
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Oct 5, 2017
@abadger
Copy link
Contributor

abadger commented Oct 5, 2017

Fixes #30836
Supercedes #31347

This looks good to me.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 5, 2017
abadger
abadger reviewed Oct 5, 2017
View reviewed changes
lib/ansible/module_utils/basic.py Outdated
msg = "Argument 'args' to run_command must be list or string"
self.fail_json(rc=257, cmd=args, msg=msg)

# expand shellisms
args = [os.path.expanduser(os.path.expandvars(x)) for x in args if x is not None]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is safe. If someone is calling this function without unsafe_shell they probably have not quoted the arguments to prevent this sort of expansion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, looking further down, I see that you're just moving this around though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a change in the end as we did not do this for stringified version .. but we probably should have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the shell itself would have done it before. but might have done it slightly differently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so with last change to always stringify in unsafe, moving this back down into an else:

abadger
abadger reviewed Oct 5, 2017
View reviewed changes
lib/ansible/module_utils/basic.py Outdated
else:
shell = True
if self._shell not in (None, '/bin/sh'):
args = [self._shell, '-c'] + args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong. Won't this end up being the equivalent of:

/bin/sh -c if [ x"test" = x"test" ] ; then printf "hi" ; fi

When what we really want is the former which is the equivalent of:

 /bin/sh -c 'if [ x"test" = x"test" ] ; then printf "hi" ; fi'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to always stringify

This was referenced Oct 5, 2017
Closed
Closed
Closed
@abadger
Copy link
Contributor

abadger commented Oct 5, 2017

This looks right. +1

@mattclay
Copy link
Member

mattclay commented Oct 5, 2017

CI failure in test/integration/targets/command_shell/tasks/main.yml:287:

{
    "assertion": "shell_result5.stdout == '5575bb6b71c9558db0b6fbbf2f19909eeb4e3b98\nthis is a second line'", 
    "changed": false, 
    "evaluated_to": false, 
    "failed": true
}

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Oct 5, 2017
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Oct 5, 2017
@bcoca
use configured ansible_shell_executable
2cf512f 
refine args/shell/executable hanlding
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 6, 2017
@bcoca bcoca added this to TODO: Next release in 2.4.x Blocker List Oct 6, 2017
@abadger
Copy link
Contributor

abadger commented Oct 6, 2017

+1 merge it.

@bcoca bcoca merged commit f2ade09 into ansible:devel Oct 6, 2017
@bcoca bcoca deleted the run_ansible_shell branch October 6, 2017 12:56
@bcoca bcoca mentioned this pull request Oct 16, 2017
Closed
@abadger abadger moved this from TODO: Next release to Done in 2.4.2 in 2.4.x Blocker List Oct 18, 2017
@abadger
Copy link
Contributor

abadger commented Oct 19, 2017

This has been cherry-picked into the temp-staging-post-2.4.0 branch for release in 2.4.1

@bcoca bcoca mentioned this pull request Nov 13, 2017
Closed
@alikins alikins mentioned this pull request Jan 29, 2018
Closed
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abadger abadger abadger left review comments

Assignees
No one assigned
Labels
affects_2.5 This issue/PR affects Ansible v2.5 bug This issue/PR relates to a bug. module_utils/basic plugins/action support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
No open projects
2.4.x Blocker List
Done in 2.4.2
Milestone
No milestone
5 participants
@bcoca @abadger @mattclay @samdoran @ansibot