Feature #2715

Optimization to avoid spawning shell in Kernel#system call should check for failure conditions

Added by Tomasz Wegrzanowski about 4 years ago. Updated over 1 year ago.

[ruby-core:28072]
Status:Feedback
Priority:Normal
Assignee:Akira Tanaka
Category:core
Target version:next minor

Description

=begin
This is an old issue, I think going all the way back to Perl's system().

Kernel#system is supposed to spawn shell and pass its argument to shell process just like C system(); except it's optimized for a common case of very simple argument, in which case Ruby (like Perl) simply does fork and exec of the string passed, split on whitespace.

This almost works, except shell error reporting is not duplicated. If command doesn't exist, shell would print an error message on stderr, while Ruby like Perl fails silently.

Problem is present in all versions of Ruby including 1.8.7 and 1.9.1.

To reproduce:
$ ruby -e 'system "fail"'
$ ruby -e 'system "\"\"fail"'
sh: fail: command not found

Output of both commands should be identical (or at least close enough, details of error message might differ by Unix flavor).

rbfsystem code for VMS and Windows uses different code paths, so I don't know if it's also affected or not.

I wrote a bit more about background of this issue on my blog: http://t-a-w.blogspot.com/2010/02/what-is-all-this-perl-doing-in-my-ruby.html
=end

remove-shell-invocation-optimization.patch Magnifier (5.07 KB) Akira Tanaka, 06/04/2012 10:34 PM


Related issues

Related to ruby-trunk - Feature #3426: exec doesn't allow command lines which begin with an env ... Rejected 06/11/2010
Related to ruby-trunk - Feature #4477: Kernel:exec and backtick (`) don't work for certain syste... Closed 03/07/2011

History

#1 Updated by Nobuyoshi Nakada about 4 years ago

  • Status changed from Open to Feedback

=begin

=end

#2 Updated by Nobuyoshi Nakada about 4 years ago

=begin
Hi,

At Sat, 6 Feb 2010 06:55:09 +0900,
Tomasz Wegrzanowski wrote in :

This almost works, except shell error reporting is not
duplicated. If command doesn't exist, shell would print an
error message on stderr, while Ruby like Perl fails silently.

Problem is present in all versions of Ruby including 1.8.7 and 1.9.1.

To reproduce:
$ ruby -e 'system "fail"'
$ ruby -e 'system "\"\"fail"'
sh: fail: command not found

Output of both commands should be identical (or at least
close enough, details of error message might differ by Unix
flavor).

What's that you want to do? In 1.9, the former returns nil,
while the later returns false, and spawn and Process.spawn
raise an Errno::ENOENT.

In addtion, system and spawn in 1.9 have been improved very
much, so you don't need to use sh for redirection.

# run "fail" command with no output
system("fail", out:"/dev/null", err:[:child, :out])

I wrote a bit more about background of this issue on my blog:
http://t-a-w.blogspot.com/2010/02/what-is-all-this-perl-doing-in-my-ruby.html

Please describe the rationale briefly on the BTS, not separated
blog.

--
Nobu Nakada

=end

#3 Updated by Tomasz Wegrzanowski about 4 years ago

=begin
Nobu: I want system "fail" and system "''fail" to behave identically - printing error message when command fail doesn't exist.

For Ruby's system() not to spawn shell should be just performance optimization, it should behave the same way regardless of it spawns shell (system "''fail") or not (system "fail").

That's all.
=end

#4 Updated by Yusuke Endoh almost 4 years ago

=begin
Hi,

For Ruby's system() not to spawn shell should be just performance optimization

I don't think so. Indeed, it is a feature for optimization, but
it is also spec.

Process invocation may cause significant result such as breaking
system. So changing that may cause compatibility issue.

Nobu: I want system "fail" and system "''fail" to behave identically - printing error message when command fail doesn't exist.

"Printing error message" is absolutely new feature.
I move this ticket to feature tracker, though the feature request
is really uncool for me...

--
Yusuke Endoh mame@tsg.ne.jp
=end

#5 Updated by Akira Tanaka almost 3 years ago

  • Project changed from Ruby to ruby-trunk
  • Category changed from core to core

#6 Updated by Akira Tanaka about 2 years ago

  • Description updated (diff)

I feel the optimization (avoid shell when no meta characters) can be removed.

We have many ways to avoid shell such as system(command, arg1, arg2).

And recent computers are fast enough to run command via shell.

#7 Updated by Yusuke Endoh about 2 years ago

  • Assignee set to Akira Tanaka

#8 Updated by Akira Tanaka almost 2 years ago

I wrote a patch to remove the optimization:
remove-shell-invocation-optimization.patch

However it was not good enough because it causes several test failures.

Several failures are caused by pid change which the invoked command is not
direct child process of ruby process.
(This depends on /bin/sh. If /bin/sh optimizes tail invocations,
invoked command process will be a direct child process.)

I think this is incompatible enough.

Now, I feel printing a error message suggested by taw is better solution.

#9 Updated by Motohiro KOSAKI almost 2 years ago

Hmm...

As akr-san described, some shells have tail invocation optimization. Thus, even if ruby doesn't have shell invocation optimization, caller can't assume that spawned process is neither child nor grand child.

Therefore, if some tests were fail, it is certainly test mistake. I still think we can remove the shell invocation optimization. It is a source of confusion and I dislike it.

Of cource, it shouldn't be backported 1.9.x.

#10 Updated by Akira Tanaka almost 2 years ago

As akr-san described, some shells have tail invocation optimization. Thus, even if ruby doesn't have shell invocation optimization, caller can't assume that spawned process is neither child nor grand child.

If we have shell invocation optimization (as now) and the command line is
simple, the command is spawned as a child process.

So removing the optimization breaks a program which assumes this behavior.

For example, IO.popen("foo bar") {|io| ... Process.kill sig, io.pid ... }
will be broken by removing the optimization.

I feel this is incompatible enough.

#11 Updated by Motohiro KOSAKI almost 2 years ago

(6/5/12 3:55 PM), akr (Akira Tanaka) wrote:

Issue #2715 has been updated by akr (Akira Tanaka).

As akr-san described, some shells have tail invocation optimization. Thus, even if ruby doesn't have shell invocation optimization, caller can't assume that spawned process is neither child nor grand child.

If we have shell invocation optimization (as now) and the command line is
simple, the command is spawned as a child process.

So removing the optimization breaks a program which assumes this behavior.

For example, IO.popen("foo bar") {|io| ... Process.kill sig, io.pid ... }
will be broken by removing the optimization.

I feel this is incompatible enough.

.... I realized IO.pid is crazy mistaken feature. It wouldn't work when using
complex cmdline even if we will not remove the optimization. Sigh. (+_+

#12 Updated by Akira Tanaka almost 2 years ago

.... I realized IO.pid is crazy mistaken feature. It wouldn't work when using complex cmdline even if we will not remove the optimization. Sigh. (+_+

Killing a complex command line, which invokes many processes, is a different problem.
IO#pid is not a origin of the problem.
spawn also have same problem as follows.

pid = spawn("foo & bar"); Process.kill(:TERM, pid)

Do you say that returning pid feature of spawn is crazy mistaken?

Killing a complex command line needs process group,
as shell job control does.

To return to this issue, I'm considering adding a new spawn option, :shell.
This option disables the optimization.

system("simple command", :shell=>true)

The default of the option is disabled as now.

This option solves this issue as:
- shell error messages are printed if :shell => true.
- doesn't break programs which expects simple commands are direct child process.
- don't need to implement mimic of shell error message printing feature in ruby.

#13 Updated by Motohiro KOSAKI almost 2 years ago

On Fri, Jun 8, 2012 at 3:21 AM, akr (Akira Tanaka) akr@fsij.org wrote:

Issue #2715 has been updated by akr (Akira Tanaka).

.... I realized IO.pid is crazy mistaken feature. It wouldn't work when using complex cmdline even if we will not remove the optimization. Sigh. (+_+

Killing a complex command line, which invokes many processes, is a different problem.
IO#pid is not a origin of the problem.
spawn also have same problem as follows.

 pid = spawn("foo & bar"); Process.kill(:TERM, pid)

Do you say that returning pid feature of spawn is crazy mistaken?

Killing a complex command line needs process group,
as shell job control does.

To return to this issue, I'm considering adding a new spawn option, :shell.
This option disables the optimization.

 system("simple command", :shell=>true)

The default of the option is disabled as now.

This option solves this issue as:
- shell error messages are printed if :shell => true.
- doesn't break programs which expects simple commands are direct child process.
- don't need to implement mimic of shell error message printing feature in ruby.

Seems make sense.

#14 Updated by Yusuke Endoh over 1 year ago

  • Target version set to next minor

Also available in: Atom PDF