[chef-dev] Re: Re: gem_package (and shellout generally) on windows

[Daniel DeLeo < >]

On Friday, November 2, 2012 at 1:18 AM, Holger Just wrote:

Hi Joseph,

On 2012-11-02 04:15, Joseph Holsten wrote:

But shellout isn't quoting or escaping (correctly?). I just tried

looking at mixlib-shellout/lib/mixlib/shellout/windows.rb, but

clearly I'm out of my depth trying to grok win32/process.

https://github.com/opscode/chef/blob/master/lib/chef/provider/package/rubygems.rb#L505

Should that command be quoted before it gets to shellout?

Should whitespace be escaped instead? Should shellout handle it

transparently?

Unfortunately, you can't just quote any whitespace you find in

shell_out! anymore as you can't know which space should be quoted

(inside the paths) and which shouldn't (between arguments).

I personally see two usable alternatives for actually fixing that:

1) You state in the documentation that every variable part of a shelled

out command should be quoted by the user. For Linux, you could use the

builtin Shellwords.escape [1] which escapes strings for the Bourne

shell. I'm sure there is a similar module for the Windows shell

somewhere. This change then would need to be applied in every invocation

of that method.

This is the intended current behavior. The documentation[0] implies this but does not state it explicitly. (see comment below)

 

2) You extend the shell_out! method (and all similar ones) to optionally

accept an Array as the first argument instead of a String. Each element

in that array would represent a distinguished argument (including the

actual executable). Inside shell_out! you can then individually quote

each argument and safely concat them for execution. That way, each

consumer of the function can use it in a way to make it much more safe

without having to reason about quotes on each and every invocation.

 

While I'd tremendously prefer the second option, it has a tiny little

downside as it would still require some thoughts about how to handle

pipes (though I honestly don't know if it is supported right now at all)

On Unix, Mixlib::ShellOut passes its arguments directly to Kernel.exec[1]. A single string gets passed to `sh -c`, while an Array will use a direct call to the exec system call. The ShellOut mixin passes arguments directly to Mixlib::ShellOut, so what you propose would be a behavior change; though I don't know of any internal Chef code that uses the Array form, so it might not be a big deal.

As a source of inspiration, I'd like to point to the cocaine gem [2]

which nicely encapsulates shell outs and handles the basic case in just

a few lines of code.

I looked at alternatives, including this one, before I wrote the original Chef::ShellOut. At the time, it only used backticks to execute commands, which does not meet Chef's use case. Looks like they've improved it quite a bit since then, but still don't have some features that we rely on in Chef, like timeouts and capturing stderr. One nice thing that they've added is support for the posix-spawn gem, which uses vfork and works on modern versions of JRuby (with C extension support). I'd like to have a posix-spawn backend for Mixlib::ShellOut but I haven't had time to write it.

 

--Holger

[1]

http://www.ruby-doc.org/stdlib-1.9.3/libdoc/shellwords/rdoc/Shellwords.html

[2] https://github.com/thoughtbot/cocaine

-- 

Daniel DeLeo

0. https://github.com/opscode/mixlib-shellout/blob/master/lib/mixlib/shellout.rb

1. http://www.ruby-doc.org/core-1.9.3/Kernel.html#method-i-exec