Jira (FACT-2054) Facter::Core::Execution.execute incorrectly expands shell builtins

0 views
Skip to first unread message

Charlie Sharpsteen (JIRA)

unread,
Oct 3, 2019, 10:04:03 AM10/3/19
to puppe...@googlegroups.com
Charlie Sharpsteen created an issue
 
Facter / Bug FACT-2054
Facter::Core::Execution.execute incorrectly expands shell builtins
Issue Type: Bug Bug
Assignee: Unassigned
Created: 2019/10/03 7:03 AM
Priority: Normal Normal
Reporter: Charlie Sharpsteen

When executing commands, Facter will expand the first word in the command string to be a fully qualified path. I.e. ls -l will become /usr/bin/ls -l. If the command string is a compound command that contains a pipeline or looping construct, then Facter will wrap the executing in /bin/sh -c.

However, the first word of the compound is still expanded to an absolute path. This breaks shell builtins like cd as they are expanded to external commands like /usr/bin/cd or fail to be found on the PATH.

Reproduction Case

  • Install the latest version of puppet-agent on CentOS 7, along with strace:

yum install -y http://yum.puppetlabs.com/puppet-release-el-7.noarch.rpm
yum install -y puppet-agent strace

  • Create a test script that loads Facter, and uses it to execute a compound command that begins with cd:

cat <<EOF > test.rb
#!/opt/puppetlabs/puppet/bin/ruby
require 'facter'
 
puts Facter::Core::Execution.execute('cd /opt/puppetlabs && ls')
EOF
chmod +x test.rb

  • Execute the test scrip.

Outcome

The script prints the contents of the current working directory instead of /opt/puppetlabs:

# ./test.rb
1
anaconda-ks.cfg
linux.iso
test.rb

Running the script under strrace reveals that cd is being expanded to /usr/bin/cd before being passed to sh -c:

# strace -f -e trace=execve ./test.rb
execve("./test.rb", ["./test.rb"], [/* 23 vars */]) = 0
strace: Process 20373 attached
strace: Process 20374 attached
[pid 20374] execve("/usr/bin/sh", ["sh", "-c", "/usr/bin/cd /opt/puppetlabs && l"...], [/* 24 vars */]) = 0
strace: Process 20375 attached
[pid 20375] execve("/usr/bin/cd", ["/usr/bin/cd", "/opt/puppetlabs"], [/* 24 vars */]) = 0
[pid 20375] +++ exited with 0 +++
[pid 20374] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20375, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
strace: Process 20376 attached
[pid 20376] execve("/usr/bin/ls", ["ls"], [/* 24 vars */]) = 0
[pid 20376] +++ exited with 0 +++
[pid 20374] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20376, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
[pid 20374] +++ exited with 0 +++
[pid 20372] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20374, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
1
anaconda-ks.cfg
linux.iso
test.rb
[pid 20373] +++ exited with 0 +++
+++ exited with 0 +++

Expected Outcome

The script prints the content of /opt/puppetlabs:

# ./test.rb
bin
facter
puppet
pxp-agent

Suggested Workaround

The expansion only affects the first word in the command line, so adding an extra true && to the compound command acts as a sacrificial noop that takes the hit instead.

Add Comment Add Comment
 
This message was sent by Atlassian JIRA (v7.7.1#77002-sha1:e75ca93)
Atlassian logo

Charlie Sharpsteen (JIRA)

unread,
Oct 3, 2019, 10:04:04 AM10/3/19
to puppe...@googlegroups.com
Charlie Sharpsteen updated an issue
Change By: Charlie Sharpsteen
When executing commands, Facter will expand the first word in the command string to be a fully qualified path. I.e. {{ls -l}} will become {{/usr/bin/ls -l}}. If the command string is a compound command that contains a pipeline or looping construct, then Facter will wrap the executing in {{/bin/sh -c}}.


However, the first word of the compound is still expanded to an absolute path. This breaks shell builtins like {{cd}} as they are expanded to external commands like {{/usr/bin/cd}} or fail to be found on the PATH.

h2. Reproduction Case

- Install the latest version of {{puppet-agent}} on CentOS 7, along with strace:

{code:bash}

yum install -y http://yum.puppetlabs.com/puppet-release-el-7.noarch.rpm
yum install -y puppet-agent strace
{code}

- Create a test script that loads Facter, and uses it to execute a compound command that begins with {{cd}}:

{code:bash}

cat <<EOF > test.rb
#!/opt/puppetlabs/puppet/bin/ruby
require 'facter'

puts Facter::Core::Execution.execute('cd /opt/puppetlabs && ls')
EOF
chmod +x test.rb
{code}

  - Execute the test
scrip script .

h3. Outcome


The script prints the contents of the current working directory instead of {{/opt/puppetlabs}}:

{noformat}

# ./test.rb
1
anaconda-ks.cfg
linux.iso
test.rb
{noformat}


Running the script under strrace reveals that {{cd}} is being expanded to {{/usr/bin/cd}} before being passed to {{sh -c}}:

{noformat}

# strace -f -e trace=execve ./test.rb
execve("./test.rb", ["./test.rb"], [/* 23 vars */]) = 0
strace: Process 20373 attached
strace: Process 20374 attached
[pid 20374] execve("/usr/bin/sh", ["sh", "-c", "/usr/bin/cd /opt/puppetlabs && l"...], [/* 24 vars */]) = 0
strace: Process 20375 attached
[pid 20375] execve("/usr/bin/cd", ["/usr/bin/cd", "/opt/puppetlabs"], [/* 24 vars */]) = 0
[pid 20375] +++ exited with 0 +++
[pid 20374] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20375, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
strace: Process 20376 attached
[pid 20376] execve("/usr/bin/ls", ["ls"], [/* 24 vars */]) = 0
[pid 20376] +++ exited with 0 +++
[pid 20374] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20376, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
[pid 20374] +++ exited with 0 +++
[pid 20372] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20374, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
1
anaconda-ks.cfg
linux.iso
test.rb
[pid 20373] +++ exited with 0 +++
+++ exited with 0 +++
{noformat}

h3. Expected Outcome


The script prints the content of {{/opt/puppetlabs}}:

{noformat}

# ./test.rb
bin
facter
puppet
pxp-agent
{noformat}

h3. Suggested Workaround


The expansion only affects the first word in the command line, so adding an extra {{true &&}} to the compound command acts as a sacrificial noop that takes the hit instead.

Charlie Sharpsteen (JIRA)

unread,
Oct 3, 2019, 10:05:03 AM10/3/19
to puppe...@googlegroups.com
Charlie Sharpsteen updated an issue
When executing commands, Facter will expand the first word in the command string to be a fully qualified path. I.e. {{ls -l}} will become {{/usr/bin/ls -l}}. If the command string is a compound command that contains a pipeline or looping conditional construct, then Facter will wrap the executing string in {{/ usr/ bin/sh -c}}.


However, the first word of the compound is still expanded to an absolute path. This breaks shell builtins like {{cd}} as they are expanded to external commands like {{/usr/bin/cd}} or fail to be found on the PATH.

h2. Reproduction Case

- Install the latest version of {{puppet-agent}} on CentOS 7, along with strace:

{code:bash}
yum install -y http://yum.puppetlabs.com/puppet-release-el-7.noarch.rpm
yum install -y puppet-agent strace
{code}

- Create a test script that loads Facter, and uses it to execute a compound command that begins with {{cd}}:

{code:bash}
cat <<EOF > test.rb
#!/opt/puppetlabs/puppet/bin/ruby
require 'facter'

puts Facter::Core::Execution.execute('cd /opt/puppetlabs && ls')
EOF
chmod +x test.rb
{code}

  - Execute the test script.

Charlie Sharpsteen (JIRA)

unread,
Oct 3, 2019, 10:08:04 AM10/3/19
to puppe...@googlegroups.com

Mihai Buzgau (JIRA)

unread,
Oct 8, 2019, 8:49:03 AM10/8/19
to puppe...@googlegroups.com

Mihai Buzgau (JIRA)

unread,
Oct 16, 2019, 4:57:03 AM10/16/19
to puppe...@googlegroups.com

Mihai Buzgau (JIRA)

unread,
Oct 16, 2019, 4:57:03 AM10/16/19
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau
Sprint: PR NW - Triage 2019-10-30

Gabriel Nagy (JIRA)

unread,
Oct 17, 2019, 8:07:03 AM10/17/19
to puppe...@googlegroups.com
Gabriel Nagy commented on Bug FACT-2054
 
Re: Facter::Core::Execution.execute incorrectly expands shell builtins

The expansion is done here: https://github.com/puppetlabs/leatherman/blob/master/execution/src/posix/execution.cc#L142

The question is what should we do here? Keep a list of sh builtins which we should check against when expanding the first argument?

Charlie Sharpsteen (JIRA)

unread,
Oct 17, 2019, 5:23:03 PM10/17/19
to puppe...@googlegroups.com

Hmm, if we're executing everything through /usr/bin/sh -c, then what are we gaining by expanding the path to the first argument? Shouldn't the shell be able to handle that by its self?

Thomas Kishel (JIRA)

unread,
Oct 21, 2019, 1:52:03 PM10/21/19
to puppe...@googlegroups.com

George Mrejea (JIRA)

unread,
Oct 22, 2019, 4:57:03 AM10/22/19
to puppe...@googlegroups.com

George Mrejea (JIRA)

unread,
Oct 23, 2019, 5:04:04 AM10/23/19
to puppe...@googlegroups.com
George Mrejea commented on Bug FACT-2054
 
Re: Facter::Core::Execution.execute incorrectly expands shell builtins

Thomas Kishel the compgen command is not available on all the platforms, so don't think we can use as a default way to determine the built-in commands. Eg Debian 10:

georges-mbp:leatherman george.mrejea$ floaty ssh debian-10-x86_64
The authenticity of host 'kqrazizpal3h2pw.delivery.puppetlabs.net (10.16.127.117)' can't be established.
ECDSA key fingerprint is SHA256:vKputyJwe1oct3MvZLSqMDA3wkyWcx/6nrEHoGJWL7c.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added 'kqrazizpal3h2pw.delivery.puppetlabs.net,10.16.127.117' (ECDSA) to the list of known hosts.
Linux kqrazizpal3h2pw 4.19.0-5-amd64 #1 SMP Debian 4.19.37-5 (2019-06-19) x86_64The programs included with the Debian GNU/Linux system are free software;
the exact distribution terms for each program are described in the
individual files in /usr/share/doc/*/copyright.Debian GNU/Linux comes with ABSOLUTELY NO WARRANTY, to the extent
permitted by applicable law.
root@kqrazizpal3h2pw:~# sh -c "compgen -b"
sh: 1: compgen: not found 

 

Charlie Sharpsteen removing the expansion would change existing functionality and it is very hard to asses the full impact. A possible solution to this would be to add another optional boolean parameter to 

Facter::Core::Execution.execute, eg. expandPath that would default to true. If set to false it would not expand the paths.

Gabriel Nagy thoughts? 

 

Gabriel Nagy (JIRA)

unread,
Oct 23, 2019, 5:13:03 AM10/23/19
to puppe...@googlegroups.com
Gabriel Nagy commented on Bug FACT-2054

compgen is bash-specific, so it does not help us here, since leatherman uses sh -c for all shell outs (https://github.com/puppetlabs/leatherman/blob/master/execution/src/posix/execution.cc#L38)

Removing the expansion altogether may act weird because of how leatherman searches in the path and how sh inherits environment variables, etc etc. I'm a bit afraid to do that .

I agree with adding a configurable option that defaults to the current behavior, and when set to false it does not expand the first argument.

Thomas Kishel (JIRA)

unread,
Oct 23, 2019, 12:27:04 PM10/23/19
to puppe...@googlegroups.com

Charlie Sharpsteen (JIRA)

unread,
Oct 24, 2019, 11:18:03 AM10/24/19
to puppe...@googlegroups.com

Adding a boolean option to disable expansion seems reasonable to me.

Mihai Buzgau (JIRA)

unread,
Oct 30, 2019, 5:00:12 AM10/30/19
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau
Sprint: NW - 2019-10-30 , NW - 2019-11-13

Mihai Buzgau (JIRA)

unread,
Nov 14, 2019, 5:28:08 AM11/14/19
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau
Sprint: NW - 2019-10-30, NW - 2019-11-13 , 2019-11-27

Mihai Buzgau (JIRA)

unread,
Nov 27, 2019, 4:51:09 AM11/27/19
to puppe...@googlegroups.com
Mihai Buzgau updated an issue
Change By: Mihai Buzgau
Sprint: NW - 2019-10-30, NW - 2019-11-13, 2019-11-27 , 2019-12-11

George Mrejea (JIRA)

unread,
Nov 29, 2019, 7:20:04 AM11/29/19
to puppe...@googlegroups.com
George Mrejea updated an issue
Change By: George Mrejea
Release Notes Summary: Facter::Core::Execution.execute by default searches command passed as argument in a list of directories and expands it to absolute path. Now accepts a new boolean parameter -expand. When true, behaves like before, searches the command and expands it to absolute path. When set to false e.g
```Facter::Core::Execution.execute(command, {:expand => false}) ```
verifies if it is a shel builtin command, and in that case command is passed as it is (not expanded to absolute path).
Release Notes: Enhancement

Dorin Pleava (JIRA)

unread,
Jan 8, 2020, 2:54:04 AM1/8/20
to puppe...@googlegroups.com

Dorin Pleava (JIRA)

unread,
Jan 8, 2020, 9:47:03 AM1/8/20
to puppe...@googlegroups.com

Jean Bond (JIRA)

unread,
Jan 14, 2020, 1:00:04 PM1/14/20
to puppe...@googlegroups.com
Jean Bond commented on Bug FACT-2054
 
Re: Facter::Core::Execution.execute incorrectly expands shell builtins

It's not clear to me how the user should interact with this and how they change this setting. Is it a flag on the command line, a setting in a config file, or something else? Gabriel Nagy, Charlie Sharpsteen?

George Mrejea (JIRA)

unread,
Jan 15, 2020, 3:53:03 AM1/15/20
to puppe...@googlegroups.com

Jean Bond This ticket modifies existing API by adding an optional parameter, allowing users to use shell builtin commands as first command in custom fact creation.

For example, minimal custom fact comprise

#!/opt/puppetlabs/puppet/bin/ruby
require 'facter'
Facter::Core::Execution.execute('first_command parameters && second_command parameters ....')

By default, puppet expands `first_command` to absolute path, searching first_command in a list of directories.
On Linux platform, users can use in place of `first_command` a shell builtin command. Builtin commands are contained within the shell itself, therefore do not have an absolute path. The shell executes the command directly, without invoking another program.
By placing {:expand => false}, user change default behaviour and expansion to an absolute path is stopped, and custom fact will execute successfully.
```Facter::Core::Execution.execute('first_command parameters && second_command parameters ....', {:expand => false}) ```
If user does not use {:expand => false}, it is equivalent to {:expand => true}, and `first_command` will be expanded to a non existent absolute path, and custom fact will fail.
Before users where forced to use as workaround a dummy command 'true' in place of `first_command`.

Gheorghe Popescu (JIRA)

unread,
Jan 15, 2020, 5:42:03 AM1/15/20
to puppe...@googlegroups.com

Jean Bond (JIRA)

unread,
Jan 15, 2020, 1:55:04 PM1/15/20
to puppe...@googlegroups.com
Jean Bond updated an issue
Change By: Jean Bond
Labels: resolved-issue-added
Reply all
Reply to author
Forward
0 new messages