* [ruby-core:91216] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
[not found] <redmine.issue-15555.20190122101131@ruby-lang.org>
@ 2019-01-22 10:11 ` kabogtob
2019-01-22 12:23 ` [ruby-core:91217] " zn
` (3 subsequent siblings)
4 siblings, 0 replies; 5+ messages in thread
From: kabogtob @ 2019-01-22 10:11 UTC (permalink / raw)
To: ruby-core
Issue #15555 has been reported by kbogtob (Karim Bogtob).
----------------------------------------
Bug #15555: Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
https://bugs.ruby-lang.org/issues/15555
* Author: kbogtob (Karim Bogtob)
* Status: Open
* Priority: Normal
* Assignee:
* Target version:
* ruby -v: ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]
* Backport: 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN
----------------------------------------
The current implementation of the Dir.mktmpdir is the following:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
if block_given?
begin
yield path
ensure
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
FileUtils.remove_entry path
end
else
path
end
end
```
When used explicitly with a directory, it raises an ArgumentError when the directory is world writable (File::Stat#world_writable?) but not sticky (File::Stat#sticky?). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an ArgumentError only when using it with the block form. Worse, it raises the error in the ensure block instead of the first thing done in the method. So it raises the ArgumentError **after** it yields to the block, so after the job is already done.
Proof:
```ruby
irb(main):001:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 2 root 1000090000 6 Jan 22 10:02 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
=> nil
irb(main):002:0> # /tmp is non sticky but world writable
irb(main):003:0* Dir.mktmpdir(nil, '/tmp') do |tmpdir|
irb(main):004:1* puts "Hello, I'm using #{tmpdir}"
irb(main):005:1> end
Hello, I'm using /tmp/d20190122-26-5biuz2
ArgumentError: parent directory is world writable but not sticky
from /opt/rh/rh-ruby24/root/usr/share/ruby/tmpdir.rb:93:in `mktmpdir'
from (irb):3
from /opt/rh/rh-ruby24/root/usr/bin/irb:11:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `kernel_load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:362:in `exec'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:22:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:13:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:30:in `block in <top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/friendly_errors.rb:121:in `with_friendly_errors'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:22:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `<main>'
irb(main):006:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 3 root 1000090000 33 Jan 22 10:03 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
drwx--S---. 2 1000090000 1000090000 6 Jan 22 10:03 d20190122-26-5biuz2
=> nil
```
We can also see that the allocated directory **remains** after the exception is raised as the implementation raised in the ensure block.
An advisable implementation would be:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
if block_given?
begin
yield path
ensure
FileUtils.remove_entry path
end
else
path
end
end
```
This way, it will make the checks first and raise directly. And no temporary directory will be leaked with the block form.
As there is no maintainer for this lib, I can make a pull request with the following changes (with unit tests) if I'm asked to.
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ruby-core:91217] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
[not found] <redmine.issue-15555.20190122101131@ruby-lang.org>
2019-01-22 10:11 ` [ruby-core:91216] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir kabogtob
@ 2019-01-22 12:23 ` zn
2019-01-29 9:20 ` [ruby-core:91316] " naruse
` (2 subsequent siblings)
4 siblings, 0 replies; 5+ messages in thread
From: zn @ 2019-01-22 12:23 UTC (permalink / raw)
To: ruby-core
Issue #15555 has been updated by znz (Kazuhiro NISHIYAMA).
https://github.com/ruby/ruby/commit/bcb9e567c422f535b4871ce2795179af808d0077 changes from `FileUtils.remove_entry_secure` to checking stat and `FileUtils.remove_entry`.
So I think splitting checking and `FileUtils.remove_entry` is not good.
----------------------------------------
Bug #15555: Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
https://bugs.ruby-lang.org/issues/15555#change-76456
* Author: kbogtob (Karim Bogtob)
* Status: Open
* Priority: Normal
* Assignee:
* Target version:
* ruby -v: ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]
* Backport: 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN
----------------------------------------
The current implementation of the Dir.mktmpdir is the following:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
if block_given?
begin
yield path
ensure
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
FileUtils.remove_entry path
end
else
path
end
end
```
When used explicitly with a directory, it raises an ArgumentError when the directory is world writable (File::Stat#world_writable?) but not sticky (File::Stat#sticky?). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an ArgumentError only when using it with the block form. Worse, it raises the error in the ensure block instead of the first thing done in the method. So it raises the ArgumentError **after** it yields to the block, so after the job is already done.
Proof:
```ruby
irb(main):001:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 2 root 1000090000 6 Jan 22 10:02 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
=> nil
irb(main):002:0> # /tmp is non sticky but world writable
irb(main):003:0* Dir.mktmpdir(nil, '/tmp') do |tmpdir|
irb(main):004:1* puts "Hello, I'm using #{tmpdir}"
irb(main):005:1> end
Hello, I'm using /tmp/d20190122-26-5biuz2
ArgumentError: parent directory is world writable but not sticky
from /opt/rh/rh-ruby24/root/usr/share/ruby/tmpdir.rb:93:in `mktmpdir'
from (irb):3
from /opt/rh/rh-ruby24/root/usr/bin/irb:11:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `kernel_load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:362:in `exec'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:22:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:13:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:30:in `block in <top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/friendly_errors.rb:121:in `with_friendly_errors'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:22:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `<main>'
irb(main):006:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 3 root 1000090000 33 Jan 22 10:03 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
drwx--S---. 2 1000090000 1000090000 6 Jan 22 10:03 d20190122-26-5biuz2
=> nil
```
We can also see that the allocated directory **remains** after the exception is raised as the implementation raised in the ensure block.
An advisable implementation would be:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
FileUtils.remove_entry path
raise ArgumentError, "parent directory is world writable but not sticky"
end
if block_given?
begin
yield path
ensure
FileUtils.remove_entry path
end
else
path
end
end
```
This way, it will make the checks first and raise directly. And no temporary directory will be leaked with the block form.
As there is no maintainer for this lib, I can make a pull request with the following changes (with unit tests) if I'm asked to.
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ruby-core:91316] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
[not found] <redmine.issue-15555.20190122101131@ruby-lang.org>
2019-01-22 10:11 ` [ruby-core:91216] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir kabogtob
2019-01-22 12:23 ` [ruby-core:91217] " zn
@ 2019-01-29 9:20 ` naruse
2019-02-28 14:55 ` [ruby-core:91644] " usa
2019-03-13 0:21 ` [ruby-core:91804] " nagachika00
4 siblings, 0 replies; 5+ messages in thread
From: naruse @ 2019-01-29 9:20 UTC (permalink / raw)
To: ruby-core
Issue #15555 has been updated by naruse (Yui NARUSE).
Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: REQUIRED to 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE
ruby_2_6 r66941 merged revision(s) 66909.
----------------------------------------
Bug #15555: Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
https://bugs.ruby-lang.org/issues/15555#change-76566
* Author: kbogtob (Karim Bogtob)
* Status: Closed
* Priority: Normal
* Assignee:
* Target version:
* ruby -v: ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]
* Backport: 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE
----------------------------------------
The current implementation of the `Dir.mktmpdir` is the following:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
if block_given?
begin
yield path
ensure
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
FileUtils.remove_entry path
end
else
path
end
end
```
When used explicitly with a directory, it raises an `ArgumentError` when the directory is world writable (`File::Stat#world_writable?`) but not sticky (`File::Stat#sticky?`). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an `ArgumentError` only when using it with the block form. Worse, it raises the error in the ensure block instead of the first thing done in the method. So it raises the `ArgumentError` **after** it yields to the block, so after the job is already done.
Proof:
```ruby
irb(main):001:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 2 root 1000090000 6 Jan 22 10:02 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
=> nil
irb(main):002:0> # /tmp is non sticky but world writable
irb(main):003:0* Dir.mktmpdir(nil, '/tmp') do |tmpdir|
irb(main):004:1* puts "Hello, I'm using #{tmpdir}"
irb(main):005:1> end
Hello, I'm using /tmp/d20190122-26-5biuz2
ArgumentError: parent directory is world writable but not sticky
from /opt/rh/rh-ruby24/root/usr/share/ruby/tmpdir.rb:93:in `mktmpdir'
from (irb):3
from /opt/rh/rh-ruby24/root/usr/bin/irb:11:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `kernel_load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:362:in `exec'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:22:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:13:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:30:in `block in <top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/friendly_errors.rb:121:in `with_friendly_errors'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:22:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `<main>'
irb(main):006:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 3 root 1000090000 33 Jan 22 10:03 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
drwx--S---. 2 1000090000 1000090000 6 Jan 22 10:03 d20190122-26-5biuz2
=> nil
```
We can also see that the allocated directory **remains** after the exception is raised as the implementation raised in the ensure block.
An advisable implementation would be:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
FileUtils.remove_entry path
raise ArgumentError, "parent directory is world writable but not sticky"
end
if block_given?
begin
yield path
ensure
FileUtils.remove_entry path
end
else
path
end
end
```
This way, it will make the checks first and raise directly. And no temporary directory will be leaked with the block form.
As there is no maintainer for this lib, I can make a pull request with the following changes (with unit tests) if I'm asked to.
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ruby-core:91644] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
[not found] <redmine.issue-15555.20190122101131@ruby-lang.org>
` (2 preceding siblings ...)
2019-01-29 9:20 ` [ruby-core:91316] " naruse
@ 2019-02-28 14:55 ` usa
2019-03-13 0:21 ` [ruby-core:91804] " nagachika00
4 siblings, 0 replies; 5+ messages in thread
From: usa @ 2019-02-28 14:55 UTC (permalink / raw)
To: ruby-core
Issue #15555 has been updated by usa (Usaku NAKAMURA).
Backport changed from 2.4: REQUIRED, 2.5: REQUIRED, 2.6: DONE to 2.4: DONE, 2.5: REQUIRED, 2.6: DONE
ruby_2_4 r67148 merged revision(s) 66909.
----------------------------------------
Bug #15555: Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
https://bugs.ruby-lang.org/issues/15555#change-76908
* Author: kbogtob (Karim Bogtob)
* Status: Closed
* Priority: Normal
* Assignee:
* Target version:
* ruby -v: ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]
* Backport: 2.4: DONE, 2.5: REQUIRED, 2.6: DONE
----------------------------------------
The current implementation of the `Dir.mktmpdir` is the following:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
if block_given?
begin
yield path
ensure
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
FileUtils.remove_entry path
end
else
path
end
end
```
When used explicitly with a directory, it raises an `ArgumentError` when the directory is world writable (`File::Stat#world_writable?`) but not sticky (`File::Stat#sticky?`). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an `ArgumentError` only when using it with the block form. Worse, it raises the error in the ensure block instead of the first thing done in the method. So it raises the `ArgumentError` **after** it yields to the block, so after the job is already done.
Proof:
```ruby
irb(main):001:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 2 root 1000090000 6 Jan 22 10:02 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
=> nil
irb(main):002:0> # /tmp is non sticky but world writable
irb(main):003:0* Dir.mktmpdir(nil, '/tmp') do |tmpdir|
irb(main):004:1* puts "Hello, I'm using #{tmpdir}"
irb(main):005:1> end
Hello, I'm using /tmp/d20190122-26-5biuz2
ArgumentError: parent directory is world writable but not sticky
from /opt/rh/rh-ruby24/root/usr/share/ruby/tmpdir.rb:93:in `mktmpdir'
from (irb):3
from /opt/rh/rh-ruby24/root/usr/bin/irb:11:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `kernel_load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:362:in `exec'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:22:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:13:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:30:in `block in <top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/friendly_errors.rb:121:in `with_friendly_errors'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:22:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `<main>'
irb(main):006:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 3 root 1000090000 33 Jan 22 10:03 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
drwx--S---. 2 1000090000 1000090000 6 Jan 22 10:03 d20190122-26-5biuz2
=> nil
```
We can also see that the allocated directory **remains** after the exception is raised as the implementation raised in the ensure block.
An advisable implementation would be:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
FileUtils.remove_entry path
raise ArgumentError, "parent directory is world writable but not sticky"
end
if block_given?
begin
yield path
ensure
FileUtils.remove_entry path
end
else
path
end
end
```
This way, it will make the checks first and raise directly. And no temporary directory will be leaked with the block form.
As there is no maintainer for this lib, I can make a pull request with the following changes (with unit tests) if I'm asked to.
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 5+ messages in thread
* [ruby-core:91804] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
[not found] <redmine.issue-15555.20190122101131@ruby-lang.org>
` (3 preceding siblings ...)
2019-02-28 14:55 ` [ruby-core:91644] " usa
@ 2019-03-13 0:21 ` nagachika00
4 siblings, 0 replies; 5+ messages in thread
From: nagachika00 @ 2019-03-13 0:21 UTC (permalink / raw)
To: ruby-core
Issue #15555 has been updated by nagachika (Tomoyuki Chikanaga).
Backport changed from 2.4: DONE, 2.5: REQUIRED, 2.6: DONE to 2.4: DONE, 2.5: DONE, 2.6: DONE
ruby_2_5 r67241 merged revision(s) 66909.
----------------------------------------
Bug #15555: Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir
https://bugs.ruby-lang.org/issues/15555#change-77078
* Author: kbogtob (Karim Bogtob)
* Status: Closed
* Priority: Normal
* Assignee:
* Target version:
* ruby -v: ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux-gnu]
* Backport: 2.4: DONE, 2.5: DONE, 2.6: DONE
----------------------------------------
The current implementation of the `Dir.mktmpdir` is the following:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
if block_given?
begin
yield path
ensure
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
raise ArgumentError, "parent directory is world writable but not sticky"
end
FileUtils.remove_entry path
end
else
path
end
end
```
When used explicitly with a directory, it raises an `ArgumentError` when the directory is world writable (`File::Stat#world_writable?`) but not sticky (`File::Stat#sticky?`). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an `ArgumentError` only when using it with the block form. Worse, it raises the error in the ensure block instead of the first thing done in the method. So it raises the `ArgumentError` **after** it yields to the block, so after the job is already done.
Proof:
```ruby
irb(main):001:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 2 root 1000090000 6 Jan 22 10:02 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
=> nil
irb(main):002:0> # /tmp is non sticky but world writable
irb(main):003:0* Dir.mktmpdir(nil, '/tmp') do |tmpdir|
irb(main):004:1* puts "Hello, I'm using #{tmpdir}"
irb(main):005:1> end
Hello, I'm using /tmp/d20190122-26-5biuz2
ArgumentError: parent directory is world writable but not sticky
from /opt/rh/rh-ruby24/root/usr/share/ruby/tmpdir.rb:93:in `mktmpdir'
from (irb):3
from /opt/rh/rh-ruby24/root/usr/bin/irb:11:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:74:in `kernel_load'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli/exec.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:362:in `exec'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:22:in `dispatch'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/cli.rb:13:in `start'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:30:in `block in <top (required)>'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/lib/bundler/friendly_errors.rb:121:in `with_friendly_errors'
from /opt/rh/rh-ruby24/root/usr/local/share/gems/gems/bundler-1.15.4/exe/bundle:22:in `<top (required)>'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load'
from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `<main>'
irb(main):006:0> puts `ls -la /tmp`
total 4
drwxrwsrwx. 3 root 1000090000 33 Jan 22 10:03 .
drwxr-xr-x. 18 root root 4096 Jan 22 09:58 ..
drwx--S---. 2 1000090000 1000090000 6 Jan 22 10:03 d20190122-26-5biuz2
=> nil
```
We can also see that the allocated directory **remains** after the exception is raised as the implementation raised in the ensure block.
An advisable implementation would be:
```ruby
def Dir.mktmpdir(prefix_suffix=nil, *rest)
path = Tmpname.create(prefix_suffix || "d", *rest) {|n| mkdir(n, 0700)}
stat = File.stat(File.dirname(path))
if stat.world_writable? and !stat.sticky?
FileUtils.remove_entry path
raise ArgumentError, "parent directory is world writable but not sticky"
end
if block_given?
begin
yield path
ensure
FileUtils.remove_entry path
end
else
path
end
end
```
This way, it will make the checks first and raise directly. And no temporary directory will be leaked with the block form.
As there is no maintainer for this lib, I can make a pull request with the following changes (with unit tests) if I'm asked to.
--
https://bugs.ruby-lang.org/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-13 0:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <redmine.issue-15555.20190122101131@ruby-lang.org>
2019-01-22 10:11 ` [ruby-core:91216] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir kabogtob
2019-01-22 12:23 ` [ruby-core:91217] " zn
2019-01-29 9:20 ` [ruby-core:91316] " naruse
2019-02-28 14:55 ` [ruby-core:91644] " usa
2019-03-13 0:21 ` [ruby-core:91804] " nagachika00
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).