gem_package (and shellout generally) on windows


#1

This was fun: http://tickets.opscode.com/browse/CHEF-3579

Essentially, gem_package is trying to shellout!() to gem deep in C:\Program Files. 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 transparently?

http://josephholsten.com


#2

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.

  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)

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.

–Holger

[1]
http://www.ruby-doc.org/stdlib-1.9.3/libdoc/shellwords/rdoc/Shellwords.html
[2] https://github.com/thoughtbot/cocaine


#3

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)

  1. 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

  1. https://github.com/opscode/mixlib-shellout/blob/master/lib/mixlib/shellout.rb
  2. http://www.ruby-doc.org/core-1.9.3/Kernel.html#method-i-exec

#4

Daniel DeLeo wrote:

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.

I’m not aware of any “outside” code using that form either. In the end,
having an officially sanctioned and encouraged way for safe
shellouts would probably heavily outweigh this loss of functionality.

–Holger