Project

General

Profile

Bug #15555

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

The current implementation of the `Dir.mktmpdir` 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` ArgumentError when the directory is world writable (`File::Stat#world_writable?`) (File::Stat#world_writable?) but not sticky (`File::Stat#sticky?`). (File::Stat#sticky?). This is a normal behavior. But with how the method is written, the behavior is inconsistent and raises an `ArgumentError` 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` 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.

Back