On Friday, November 2, 2012 at 1:18 AM, Holger Just wrote:
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.
Should that command be quoted before it gets to shellout?
Should whitespace be escaped instead? Should shellout handle it
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:
- 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  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 implies this but does not state it explicitly. (see comment below)
- 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. 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 
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.