[ruby-core:91804] [Ruby trunk Bug#15555] Dir.mktmpdir checks permissions and raise ArgumentError after yielding to block (ensure) & leaks allocated tempdir

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/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>