Update Uniquefile name generation

Commit 19cf4be added `Puppet::Util::Tempfile`,
based on ruby 1.9.3p547. It was later renamed to `Uniquefile`. The code
contained a bug whereby if any TEMP, TMP, etc environment variable was defined,
but referred to a non-existent directory, then `Uniquefile.new` would raise and
never fallback to other candidate temp directories.

Ruby 2.0 added `Dir::Tmpname` as a way to generate a unique name. So just call that
and delete the problematic `make_tmpname`, etc private methods. There have also
been a number of fixes to Dir::Tmpname since 1.9.3:

* Ignore empty TMP, etc env variables
* Ignore TMP, etc env variables that refer to non-existent directories
* Warn if TMP, etc exist, but are not directories, not writable, etc
* Raising if we exhaust the list of candidate temp dirs
* Restricting allowed characters to only /,-.0-9A-Z_a-z~/

Author: joshcooper

Diff for: lib/puppet/file_system/uniquefile.rb

@@ -27,34 +27,26 @@ def self.open_tmp(identifier)
     end
   end
 
-  def initialize(basename, *rest)
-    create_tmpname(basename, *rest) do |tmpname, _n, opts|
-      mode = File::RDWR | File::CREAT | File::EXCL
-      perm = 0o600
-      if opts
-        mode |= opts.delete(:mode) || 0
-        opts[:perm] = perm
-        perm = nil
-      else
-        opts = perm
-      end
+  def initialize(basename, tmpdir = nil, mode: 0)
+    @mode = mode | File::RDWR | File::CREAT | File::EXCL
+
+    Dir::Tmpname.create(basename, tmpdir) do |tmpname, _n, opts|
+      opts[:perm] = 0600
       self.class.locking(tmpname) do
-        @tmpfile = File.open(tmpname, mode, opts)
+        @tmpfile = File.open(tmpname, @mode, **opts)
         @tmpname = tmpname
       end
-      @mode = mode & ~(File::CREAT | File::EXCL)
-      perm or opts.freeze
-      @opts = opts
+      @opts = opts.freeze
     end
 
     super(@tmpfile)
   end
 
   # Opens or reopens the file with mode "r+".
   def open
-    @tmpfile.close if @tmpfile
-    @tmpfile = File.open(@tmpname, @mode, @opts)
-    __setobj__(@tmpfile)
+    _close
+    mode = @mode & ~(File::CREAT|File::EXCL)
+    @tmpfile = File.open(@tmpname, mode, **@opts)
   end
 
   def _close
@@ -99,71 +91,8 @@ def path
 
   private
 
-  def make_tmpname(prefix_suffix, n)
-    case prefix_suffix
-    when String
-      prefix = prefix_suffix
-      suffix = ""
-    when Array
-      prefix = prefix_suffix[0]
-      suffix = prefix_suffix[1]
-    else
-      raise ArgumentError, _("unexpected prefix_suffix: %{value}") % { value: prefix_suffix.inspect }
-    end
-    t = Time.now.strftime("%Y%m%d")
-    path = "#{prefix}#{t}-#{$PROCESS_ID}-#{rand(0x100000000).to_s(36)}"
-    path << "-#{n}" if n
-    path << suffix
-  end
-
-  def create_tmpname(basename, *rest)
-    opts = try_convert_to_hash(rest[-1])
-    if opts
-      opts = opts.dup if rest.pop.equal?(opts)
-      max_try = opts.delete(:max_try)
-      opts = [opts]
-    else
-      opts = []
-    end
-    tmpdir, = *rest
-    tmpdir ||= tmpdir()
-    n = nil
-    begin
-      path = File.join(tmpdir, make_tmpname(basename, n))
-      yield(path, n, *opts)
-    rescue Errno::EEXIST
-      n ||= 0
-      n += 1
-      retry if !max_try or n < max_try
-      raise _("cannot generate temporary name using `%{basename}' under `%{tmpdir}'") % { basename: basename, tmpdir: tmpdir }
-    end
-    path
-  end
-
-  def try_convert_to_hash(h)
-    h.to_hash
-  rescue NoMethodError
-    nil
-  end
-
   @@systmpdir ||= defined?(Etc.systmpdir) ? Etc.systmpdir : '/tmp'
 
-  def tmpdir
-    tmp = '.'
-    [ENV.fetch('TMPDIR', nil), ENV.fetch('TMP', nil), ENV.fetch('TEMP', nil), @@systmpdir, '/tmp'].each do |dir|
-      stat = File.stat(dir) if dir
-      begin
-        if stat && stat.directory? && stat.writable?
-          tmp = dir
-          break
-        end
-      rescue
-        nil
-      end
-    end
-    File.expand_path(tmp)
-  end
-
   class << self
     # yields with locking for +tmpname+ and returns the result of the
     # block.

Diff for: spec/unit/file_system/uniquefile_spec.rb

@@ -241,5 +241,20 @@ def tempfile(*args, &block)
       t.close
       expect(File.size(t.path)).to eq(5)
     end
+
+    it "ignores non-existent directories" do
+      nonexistent = tmpdir('nonexistent')
+      Dir.rmdir(nonexistent)
+      valid = tmpdir('valid')
+      # assuming directories are checked in this order: TMPDIR -> TMP -> TEMP
+      Puppet::Util.withenv('TMPDIR' => nonexistent, 'TMP' => '', 'TEMP' => valid) do
+        t = tempfile("foo")
+        expect(t.path).to start_with(valid)
+      end
+    end
+
+    it "strips invalid characters" do
+      expect(File.basename(tempfile("../../foo").path)).to start_with("....foo")
+    end
   end
 end

Diff for: spec/unit/util/execution_spec.rb

@@ -838,12 +838,11 @@ def expect_cwd_to_be(cwd)
         it "should raise if it fails to create a Uniquefile for stdout" do
           stdout = Puppet::FileSystem::Uniquefile.new('test')
           allow(Puppet::FileSystem::Uniquefile).to receive(:new)
-            .and_raise(
-              Errno::ENOENT,
-              'No such file or directory @ rb_file_s_stat - C:\Users\ADMINI~1\AppData\Local\Temp\doesnotexist')
+            .and_raise(ArgumentError, 'could not find a temporary directory')
+
           expect {
             Puppet::Util::Execution.execute('test command')
-          }.to raise_error(Errno::ENOENT, 'No such file or directory @ rb_file_s_stat - C:\Users\ADMINI~1\AppData\Local\Temp\doesnotexist')
+          }.to raise_error(ArgumentError, 'could not find a temporary directory')
         end
       end