1berry/ruby
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Deprecate Kernel#open and IO support for subprocess creation/forking

Browse Source 
Deprecate Kernel#open and IO support for subprocess creation and
forking. This deprecates subprocess creation and forking in

- Kernel#open
- URI.open
- IO.binread
- IO.foreach
- IO.readlines
- IO.read
- IO.write

This behavior is slated to be removed in Ruby 4.0

[Feature #19630]
This commit is contained in:
Mike Dalessio 2023-06-07 10:05:04 -04:00 committed by Nobuyoshi Nakada
parent 984109b836
commit d2343368ab
Notes: git 2023-08-10 00:38:32 +00:00 
Merged: https://github.com/ruby/ruby/pull/7915
10 changed files with 165 additions and 180 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
doc/command_injection.rdoc
View File

@ -8,6 +8,7 @@ They should not be called with unknown or unsanitized commands.
These methods include:
- Kernel.system
- Kernel.open
- {\`command` (backtick method)}[rdoc-ref:Kernel#`]
(also called by the expression <tt>%x[command]</tt>).
- IO.popen(command).
@ -17,6 +18,7 @@ These methods include:
- IO.binwrite(command).
- IO.readlines(command).
- IO.foreach(command).
- URI.open(command).
Note that some of these methods do not execute commands when called
from subclass \File:

153
io.c
View File

@ -8109,20 +8109,10 @@ check_pipe_command(VALUE filename_or_command)
* open(path, mode = 'r', perm = 0666, **opts) -> io or nil
* open(path, mode = 'r', perm = 0666, **opts) {|io| ... } -> obj
*
* Creates an IO object connected to the given stream, file, or subprocess.
* Creates an IO object connected to the given file.
*
* Required string argument +path+ determines which of the following occurs:
*
* - The file at the specified +path+ is opened.
* - The process forks.
* - A subprocess is created.
*
* Each of these is detailed below.
*
* <b>File Opened</b>
* If +path+ does _not_ start with a pipe character (<tt>'|'</tt>),
* a file stream is opened with <tt>File.open(path, mode, perm, **opts)</tt>.
* This method has potential security vulnerabilities if called with untrusted input;
* see {Command Injection}[rdoc-ref:command_injection.rdoc].
*
* With no block given, file stream is returned:
*
@ -8139,67 +8129,6 @@ check_pipe_command(VALUE filename_or_command)
*
* See File.open for details.
*
* <b>Process Forked</b>
*
* If +path+ is the 2-character string <tt>'|-'</tt>, the process forks
* and the child process is connected to the parent.
*
* With no block given:
*
* io = open('|-')
* if io
* $stderr.puts "In parent, child pid is #{io.pid}."
* else
* $stderr.puts "In child, pid is #{$$}."
* end
*
* Output:
*
* In parent, child pid is 27903.
* In child, pid is 27903.
*
* With a block given:
*
* open('|-') do |io|
* if io
* $stderr.puts "In parent, child pid is #{io.pid}."
* else
* $stderr.puts "In child, pid is #{$$}."
* end
* end
*
* Output:
*
* In parent, child pid is 28427.
* In child, pid is 28427.
*
* <b>Subprocess Created</b>
*
* If +path+ is <tt>'|command'</tt> (<tt>'command' != '-'</tt>),
* a new subprocess runs the command; its open stream is returned.
* Note that the command may be processed by shell if it contains
* shell metacharacters.
*
* With no block given:
*
* io = open('|echo "Hi!"') # => #<IO:fd 12>
* print io.gets
* io.close
*
* Output:
*
* "Hi!"
*
* With a block given, calls the block with the stream, then closes the stream:
*
* open('|echo "Hi!"') do |io|
* print io.gets
* end
*
* Output:
*
* "Hi!"
*
*/
static VALUE
@ -8222,6 +8151,8 @@ rb_f_open(int argc, VALUE *argv, VALUE _)
else {
VALUE cmd = check_pipe_command(tmp);
if (!NIL_P(cmd)) {
// TODO: when removed in 4.0, update command_injection.rdoc
rb_warn_deprecated_to_remove_at(4.0, "Calling Kernel#open with a leading '|'", "IO.popen");
argv[0] = cmd;
return rb_io_s_popen(argc, argv, rb_cIO);
}
@ -8259,6 +8190,8 @@ rb_io_open_generic(VALUE klass, VALUE filename, int oflags, int fmode,
{
VALUE cmd;
if (klass == rb_cIO && !NIL_P(cmd = check_pipe_command(filename))) {
// TODO: when removed in 4.0, update command_injection.rdoc
rb_warn_deprecated_to_remove_at(4.0, "IO process creation with a leading '|'", "IO.popen");
return pipe_open_s(cmd, rb_io_oflags_modestr(oflags), fmode, convconfig);
}
else {
@ -11914,9 +11847,6 @@ io_s_foreach(VALUE v)
* IO.foreach(path, sep = $/, **opts) {|line| block } -> nil
* IO.foreach(path, limit, **opts) {|line| block } -> nil
* IO.foreach(path, sep, limit, **opts) {|line| block } -> nil
* IO.foreach(command, sep = $/, **opts) {|line| block } -> nil
* IO.foreach(command, limit, **opts) {|line| block } -> nil
* IO.foreach(command, sep, limit, **opts) {|line| block } -> nil
* IO.foreach(...) -> an_enumerator
*
* Calls the block with each successive line read from the stream.
@ -11925,16 +11855,7 @@ io_s_foreach(VALUE v)
* this method has potential security vulnerabilities if called with untrusted input;
* see {Command Injection}[rdoc-ref:command_injection.rdoc].
*
* The first argument must be a string that is one of the following:
*
* - Path: if +self+ is a subclass of \IO (\File, for example),
* or if the string _does_ _not_ start with the pipe character (<tt>'|'</tt>),
* the string is the path to a file.
* - Command: if +self+ is the class \IO,
* and if the string starts with the pipe character,
* the rest of the string is a command to be executed as a subprocess.
* This usage has potential security vulnerabilities if called with untrusted input;
* see {Command Injection}[rdoc-ref:command_injection.rdoc].
* The first argument must be a string that is the path to a file.
*
* With only argument +path+ given, parses lines from the file at the given +path+,
* as determined by the default line separator,
@ -12028,9 +11949,6 @@ io_s_readlines(VALUE v)
/*
* call-seq:
* IO.readlines(command, sep = $/, **opts) -> array
* IO.readlines(command, limit, **opts) -> array
* IO.readlines(command, sep, limit, **opts) -> array
* IO.readlines(path, sep = $/, **opts) -> array
* IO.readlines(path, limit, **opts) -> array
* IO.readlines(path, sep, limit, **opts) -> array
@ -12041,19 +11959,7 @@ io_s_readlines(VALUE v)
* this method has potential security vulnerabilities if called with untrusted input;
* see {Command Injection}[rdoc-ref:command_injection.rdoc].
*
* The first argument must be a string;
* its meaning depends on whether it starts with the pipe character (<tt>'|'</tt>):
*
* - If so (and if +self+ is \IO),
* the rest of the string is a command to be executed as a subprocess.
* - Otherwise, the string is the path to a file.
*
* With only argument +command+ given, executes the command in a shell,
* parses its $stdout into lines, as determined by the default line separator,
* and returns those lines in an array:
*
* IO.readlines('| cat t.txt')
* # => ["First line\n", "Second line\n", "\n", "Third line\n", "Fourth line\n"]
* The first argument must be a string that is the path to a file.
*
* With only argument +path+ given, parses lines from the file at the given +path+,
* as determined by the default line separator,
@ -12062,8 +11968,6 @@ io_s_readlines(VALUE v)
* IO.readlines('t.txt')
* # => ["First line\n", "Second line\n", "\n", "Third line\n", "Fourth line\n"]
*
* For both forms, command and path, the remaining arguments are the same.
*
* With argument +sep+ given, parses lines as determined by that line separator
* (see {Line Separator}[rdoc-ref:IO@Line+Separator]):
*
@ -12136,7 +12040,6 @@ seek_before_access(VALUE argp)
/*
* call-seq:
* IO.read(command, length = nil, offset = 0, **opts) -> string or nil
* IO.read(path, length = nil, offset = 0, **opts) -> string or nil
*
* Opens the stream, reads and returns some or all of its content,
@ -12146,18 +12049,7 @@ seek_before_access(VALUE argp)
* this method has potential security vulnerabilities if called with untrusted input;
* see {Command Injection}[rdoc-ref:command_injection.rdoc].
*
* The first argument must be a string;
* its meaning depends on whether it starts with the pipe character (<tt>'|'</tt>):
*
* - If so (and if +self+ is \IO),
* the rest of the string is a command to be executed as a subprocess.
* - Otherwise, the string is the path to a file.
*
* With only argument +command+ given, executes the command in a shell,
* returns its entire $stdout:
*
* IO.read('| cat t.txt')
* # => "First line\nSecond line\n\nThird line\nFourth line\n"
* The first argument must be a string that is the path to a file.
*
* With only argument +path+ given, reads in text mode and returns the entire content
* of the file at the given path:
@ -12169,8 +12061,6 @@ seek_before_access(VALUE argp)
* unread when encountering certain special bytes. Consider using
* IO.binread if all bytes in the file should be read.
*
* For both forms, command and path, the remaining arguments are the same.
*
* With argument +length+, returns +length+ bytes if available:
*
* IO.read('t.txt', 7) # => "First l"
@ -12221,7 +12111,6 @@ rb_io_s_read(int argc, VALUE *argv, VALUE io)
/*
* call-seq:
* IO.binread(command, length = nil, offset = 0) -> string or nil
* IO.binread(path, length = nil, offset = 0) -> string or nil
*
* Behaves like IO.read, except that the stream is opened in binary mode
@ -12326,7 +12215,6 @@ io_s_write(int argc, VALUE *argv, VALUE klass, int binary)
/*
* call-seq:
* IO.write(command, data, **opts) -> integer
* IO.write(path, data, offset = 0, **opts) -> integer
*
* Opens the stream, writes the given +data+ to it,
@ -12336,25 +12224,9 @@ io_s_write(int argc, VALUE *argv, VALUE klass, int binary)
* this method has potential security vulnerabilities if called with untrusted input;
* see {Command Injection}[rdoc-ref:command_injection.rdoc].
*
* The first argument must be a string;
* its meaning depends on whether it starts with the pipe character (<tt>'|'</tt>):
* The first argument must be a string that is the path to a file.
*
* - If so (and if +self+ is \IO),
* the rest of the string is a command to be executed as a subprocess.
* - Otherwise, the string is the path to a file.
*
* With argument +command+ given, executes the command in a shell,
* passes +data+ through standard input, writes its output to $stdout,
* and returns the length of the given +data+:
*
* IO.write('| cat', 'Hello World!') # => 12
*
* Output:
*
* Hello World!
*
* With argument +path+ given, writes the given +data+ to the file
* at that path:
* With only argument +path+ given, writes the given +data+ to the file at that path:
*
* IO.write('t.tmp', 'abc') # => 3
* File.read('t.tmp') # => "abc"
@ -12393,7 +12265,6 @@ rb_io_s_write(int argc, VALUE *argv, VALUE io)
/*
* call-seq:
* IO.binwrite(command, string, offset = 0) -> integer
* IO.binwrite(path, string, offset = 0) -> integer
*
* Behaves like IO.write, except that the stream is opened in binary mode

10
spec/ruby/core/io/binread_spec.rb
View File

@ -44,4 +44,14 @@ describe "IO.binread" do
it "raises an Errno::EINVAL when not passed a valid offset" do
-> { IO.binread @fname, 0, -1 }.should raise_error(Errno::EINVAL)
end
ruby_version_is "3.3" do
# https://bugs.ruby-lang.org/issues/19630
it "warns about deprecation given a path with a pipe" do
cmd = "|echo ok"
-> {
IO.binread(cmd)
}.should complain(/IO process creation with a leading '\|'/)
end
end
end

19
spec/ruby/core/io/foreach_spec.rb
View File

@ -20,7 +20,10 @@ describe "IO.foreach" do
platform_is :windows do
cmd = "|cmd.exe /C echo hello&echo line2"
end
IO.foreach(cmd) { |l| ScratchPad << l }
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
IO.foreach(cmd) { |l| ScratchPad << l }
end
ScratchPad.recorded.should == ["hello\n", "line2\n"]
end
@ -28,7 +31,9 @@ describe "IO.foreach" do
it "gets data from a fork when passed -" do
parent_pid = $$
IO.foreach("|-") { |l| ScratchPad << l }
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
IO.foreach("|-") { |l| ScratchPad << l }
end
if $$ == parent_pid
ScratchPad.recorded.should == ["hello\n", "from a fork\n"]
@ -39,6 +44,16 @@ describe "IO.foreach" do
end
end
end
ruby_version_is "3.3" do
# https://bugs.ruby-lang.org/issues/19630
it "warns about deprecation given a path with a pipe" do
cmd = "|echo ok"
-> {
IO.foreach(cmd).to_a
}.should complain(/IO process creation with a leading '\|'/)
end
end
end
end

34
spec/ruby/core/io/read_spec.rb
View File

@ -165,12 +165,19 @@ describe "IO.read from a pipe" do
platform_is :windows do
cmd = "|cmd.exe /C echo hello"
end
IO.read(cmd).should == "hello\n"
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
IO.read(cmd).should == "hello\n"
end
end
platform_is_not :windows do
it "opens a pipe to a fork if the rest is -" do
str = IO.read("|-")
str = nil
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
str = IO.read("|-")
end
if str # parent
str.should == "hello from child\n"
else #child
@ -185,13 +192,18 @@ describe "IO.read from a pipe" do
platform_is :windows do
cmd = "|cmd.exe /C echo hello"
end
IO.read(cmd, 1).should == "h"
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
IO.read(cmd, 1).should == "h"
end
end
platform_is_not :windows do
it "raises Errno::ESPIPE if passed an offset" do
-> {
IO.read("|sh -c 'echo hello'", 1, 1)
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
IO.read("|sh -c 'echo hello'", 1, 1)
end
}.should raise_error(Errno::ESPIPE)
end
end
@ -202,11 +214,23 @@ quarantine! do # The process tried to write to a nonexistent pipe.
# once https://bugs.ruby-lang.org/issues/12230 is fixed.
it "raises Errno::EINVAL if passed an offset" do
-> {
IO.read("|cmd.exe /C echo hello", 1, 1)
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
IO.read("|cmd.exe /C echo hello", 1, 1)
end
}.should raise_error(Errno::EINVAL)
end
end
end
ruby_version_is "3.3" do
# https://bugs.ruby-lang.org/issues/19630
it "warns about deprecation given a path with a pipe" do
cmd = "|echo ok"
-> {
IO.read(cmd)
}.should complain(/IO process creation with a leading '\|'/)
end
end
end
describe "IO.read on an empty file" do

21
spec/ruby/core/io/readlines_spec.rb
View File

@ -180,13 +180,20 @@ describe "IO.readlines" do
platform_is :windows do
cmd = "|cmd.exe /C echo hello&echo line2"
end
lines = IO.readlines(cmd)
lines = nil
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
lines = IO.readlines(cmd)
end
lines.should == ["hello\n", "line2\n"]
end
platform_is_not :windows do
it "gets data from a fork when passed -" do
lines = IO.readlines("|-")
lines = nil
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
lines = IO.readlines("|-")
end
if lines # parent
lines.should == ["hello\n", "from a fork\n"]
@ -199,6 +206,16 @@ describe "IO.readlines" do
end
end
ruby_version_is "3.3" do
# https://bugs.ruby-lang.org/issues/19630
it "warns about deprecation given a path with a pipe" do
cmd = "|echo ok"
-> {
IO.readlines(cmd)
}.should complain(/IO process creation with a leading '\|'/)
end
end
it_behaves_like :io_readlines, :readlines
it_behaves_like :io_readlines_options_19, :readlines
end

11
spec/ruby/core/io/write_spec.rb
View File

@ -215,6 +215,17 @@ describe "IO.write" do
end
end
end
ruby_version_is "3.3" do
# https://bugs.ruby-lang.org/issues/19630
it "warns about deprecation given a path with a pipe" do
-> {
-> {
IO.write("|cat", "xxx")
}.should output_to_fd("xxx")
}.should complain(/IO process creation with a leading '\|'/)
end
end
end
end

36
spec/ruby/core/kernel/open_spec.rb
View File

@ -29,7 +29,9 @@ describe "Kernel#open" do
platform_is_not :windows, :wasi do
it "opens an io when path starts with a pipe" do
@io = open("|date")
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
@io = open("|date")
end
begin
@io.should be_kind_of(IO)
@io.read
@ -39,21 +41,27 @@ describe "Kernel#open" do
end
it "opens an io when called with a block" do
@output = open("|date") { |f| f.read }
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
@output = open("|date") { |f| f.read }
end
@output.should_not == ''
end
it "opens an io for writing" do
-> do
bytes = open("|cat", "w") { |io| io.write(".") }
bytes.should == 1
end.should output_to_fd(".")
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
-> {
bytes = open("|cat", "w") { |io| io.write(".") }
bytes.should == 1
}.should output_to_fd(".")
end
end
end
platform_is :windows do
it "opens an io when path starts with a pipe" do
@io = open("|date /t")
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
@io = open("|date /t")
end
begin
@io.should be_kind_of(IO)
@io.read
@ -63,11 +71,23 @@ describe "Kernel#open" do
end
it "opens an io when called with a block" do
@output = open("|date /t") { |f| f.read }
suppress_warning do # https://bugs.ruby-lang.org/issues/19630
@output = open("|date /t") { |f| f.read }
end
@output.should_not == ''
end
end
ruby_version_is "3.3" do
# https://bugs.ruby-lang.org/issues/19630
it "warns about deprecation given a path with a pipe" do
cmd = "|echo ok"
-> {
open(cmd) { |f| f.read }
}.should complain(/Kernel#open with a leading '\|'/)
end
end
it "raises an ArgumentError if not passed one argument" do
-> { open }.should raise_error(ArgumentError)
end

27
test/ruby/test_io.rb
View File

@ -2412,15 +2412,19 @@ class TestIO < Test::Unit::TestCase
end
def test_open_pipe
open("|" + EnvUtil.rubybin, "r+") do |f|
f.puts "puts 'foo'"
f.close_write
assert_equal("foo\n", f.read)
assert_deprecated_warning(/Kernel#open with a leading '\|'/) do # https://bugs.ruby-lang.org/issues/19630
open("|" + EnvUtil.rubybin, "r+") do |f|
f.puts "puts 'foo'"
f.close_write
assert_equal("foo\n", f.read)
end
end
end
def test_read_command
assert_equal("foo\n", IO.read("|echo foo"))
assert_deprecated_warning(/IO process creation with a leading '\|'/) do # https://bugs.ruby-lang.org/issues/19630
assert_equal("foo\n", IO.read("|echo foo"))
end
assert_raise(Errno::ENOENT, Errno::EINVAL) do
File.read("|#{EnvUtil.rubybin} -e puts")
end
@ -2434,7 +2438,9 @@ class TestIO < Test::Unit::TestCase
Class.new(IO).binread("|#{EnvUtil.rubybin} -e puts")
end
assert_raise(Errno::ESPIPE) do
IO.read("|echo foo", 1, 1)
assert_deprecated_warning(/IO process creation with a leading '\|'/) do # https://bugs.ruby-lang.org/issues/19630
IO.read("|echo foo", 1, 1)
end
end
end
@ -2619,11 +2625,16 @@ class TestIO < Test::Unit::TestCase
def test_foreach
a = []
IO.foreach("|" + EnvUtil.rubybin + " -e 'puts :foo; puts :bar; puts :baz'") {|x| a << x }
assert_deprecated_warning(/IO process creation with a leading '\|'/) do # https://bugs.ruby-lang.org/issues/19630
IO.foreach("|" + EnvUtil.rubybin + " -e 'puts :foo; puts :bar; puts :baz'") {|x| a << x }
end
assert_equal(["foo\n", "bar\n", "baz\n"], a)
a = []
IO.foreach("|" + EnvUtil.rubybin + " -e 'puts :zot'", :open_args => ["r"]) {|x| a << x }
assert_deprecated_warning(/IO process creation with a leading '\|'/) do # https://bugs.ruby-lang.org/issues/19630
IO.foreach("|" + EnvUtil.rubybin + " -e 'puts :zot'", :open_args => ["r"]) {|x| a << x }
end
assert_equal(["zot\n"], a)
make_tempfile {|t|

32
test/ruby/test_io_m17n.rb
View File

@ -1396,23 +1396,27 @@ EOT
end
def test_open_pipe_r_enc
open("|#{EnvUtil.rubybin} -e 'putc 255'", "r:ascii-8bit") {|f|
assert_equal(Encoding::ASCII_8BIT, f.external_encoding)
assert_equal(nil, f.internal_encoding)
s = f.read
assert_equal(Encoding::ASCII_8BIT, s.encoding)
assert_equal("\xff".force_encoding("ascii-8bit"), s)
}
EnvUtil.suppress_warning do # https://bugs.ruby-lang.org/issues/19630
open("|#{EnvUtil.rubybin} -e 'putc 255'", "r:ascii-8bit") {|f|
assert_equal(Encoding::ASCII_8BIT, f.external_encoding)
assert_equal(nil, f.internal_encoding)
s = f.read
assert_equal(Encoding::ASCII_8BIT, s.encoding)
assert_equal("\xff".force_encoding("ascii-8bit"), s)
}
end
end
def test_open_pipe_r_enc2
open("|#{EnvUtil.rubybin} -e 'putc \"\\u3042\"'", "r:UTF-8") {|f|
assert_equal(Encoding::UTF_8, f.external_encoding)
assert_equal(nil, f.internal_encoding)
s = f.read
assert_equal(Encoding::UTF_8, s.encoding)
assert_equal("\u3042", s)
}
EnvUtil.suppress_warning do # https://bugs.ruby-lang.org/issues/19630
open("|#{EnvUtil.rubybin} -e 'putc \"\\u3042\"'", "r:UTF-8") {|f|
assert_equal(Encoding::UTF_8, f.external_encoding)
assert_equal(nil, f.internal_encoding)
s = f.read
assert_equal(Encoding::UTF_8, s.encoding)
assert_equal("\u3042", s)
}
end
end
def test_s_foreach_enc