From: usa@... Date: 2019-02-28T14:55:10+00:00 Subject: [ruby-core:91644] [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 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 `' 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 ' 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 `' from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `load' from /opt/rh/rh-ruby24/root/usr/bin/bundle:23:in `
' 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: