Ensure unique attachment filenames (#35539).

Patch by Jens Krämer.


git-svn-id: http://svn.redmine.org/redmine/trunk@21071 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/models/attachment.rb

@@ -138,14 +138,10 @@ class Attachment < ActiveRecord::Base
   def files_to_final_location
     if @temp_file
       self.disk_directory = target_directory
-      self.disk_filename = Attachment.disk_filename(filename, disk_directory)
-      logger.info("Saving attachment '#{self.diskfile}' (#{@temp_file.size} bytes)") if logger
-      path = File.dirname(diskfile)
-      unless File.directory?(path)
-        FileUtils.mkdir_p(path)
-      end
       sha = Digest::SHA256.new
-      File.open(diskfile, "wb") do |f|
+      Attachment.create_diskfile(filename, disk_directory) do |f|
+        self.disk_filename = File.basename f.path
+        logger.info("Saving attachment '#{self.diskfile}' (#{@temp_file.size} bytes)") if logger
         if @temp_file.respond_to?(:read)
           buffer = ""
           while (buffer = @temp_file.read(8192))
@@ -557,9 +553,8 @@ class Attachment < ActiveRecord::Base
 
   # Singleton class method is public
   class << self
-    # Returns an ASCII or hashed filename that do not
-    # exists yet in the given subdirectory
-    def disk_filename(filename, directory=nil)
+    # Claims a unique ASCII or hashed filename, yields the open file handle
+    def create_diskfile(filename, directory=nil, &block)
       timestamp = DateTime.now.strftime("%y%m%d%H%M%S")
       ascii = ''
       if %r{^[a-zA-Z0-9_\.\-]*$}.match?(filename) && filename.length <= 50
@@ -569,11 +564,21 @@ class Attachment < ActiveRecord::Base
         # keep the extension if any
         ascii << $1 if filename =~ %r{(\.[a-zA-Z0-9]+)$}
       end
-      while File.exist?(File.join(storage_path, directory.to_s,
-                                  "#{timestamp}_#{ascii}"))
+
+      path = File.join storage_path, directory.to_s
+      FileUtils.mkdir_p(path) unless File.directory?(path)
+      begin
+        name = "#{timestamp}_#{ascii}"
+        File.open(
+          File.join(path, name),
+          flags: File::CREAT | File::EXCL | File::BINARY | File::WRONLY,
+          binmode: true,
+          &block
+        )
+      rescue Errno::EEXIST
         timestamp.succ!
+        retry
       end
-      "#{timestamp}_#{ascii}"
     end
   end
 end

test/unit/attachment_test.rb

@@ -275,12 +275,28 @@ class AttachmentTest < ActiveSupport::TestCase
     assert_equal 'valid_[] invalid_chars', a.filename
   end
 
-  def test_diskfilename
-    assert Attachment.disk_filename("test_file.txt") =~ /^\d{12}_test_file.txt$/
-    assert_equal 'test_file.txt', Attachment.disk_filename("test_file.txt")[13..-1]
-    assert_equal '770c509475505f37c2b8fb6030434d6b.txt', Attachment.disk_filename("test_accentué.txt")[13..-1]
-    assert_equal 'f8139524ebb8f32e51976982cd20a85d', Attachment.disk_filename("test_accentué")[13..-1]
-    assert_equal 'cbb5b0f30978ba03731d61f9f6d10011', Attachment.disk_filename("test_accentué.ça")[13..-1]
+  def test_create_diskfile
+    Attachment.create_diskfile("test_file.txt") do |f|
+      path = f.path
+      assert_match(/^\d{12}_test_file.txt$/, File.basename(path))
+      assert_equal 'test_file.txt', File.basename(path)[13..-1]
+      File.unlink f.path
+    end
+
+    Attachment.create_diskfile("test_accentué.txt") do |f|
+      assert_equal '770c509475505f37c2b8fb6030434d6b.txt', File.basename(f.path)[13..-1]
+      File.unlink f.path
+    end
+
+    Attachment.create_diskfile("test_accentué") do |f|
+      assert_equal 'f8139524ebb8f32e51976982cd20a85d', File.basename(f.path)[13..-1]
+      File.unlink f.path
+    end
+
+    Attachment.create_diskfile("test_accentué.ça") do |f|
+      assert_equal 'cbb5b0f30978ba03731d61f9f6d10011', File.basename(f.path)[13..-1]
+      File.unlink f.path
+    end
   end
 
   def test_title