Fix atomic file writes exposing empty destinations and leaking temp files

lib/rubygems/util/atomic_file_writer.rb

@@ -24,7 +24,7 @@ def self.open(file_name)
       tmp_suffix = ".tmp.#{SecureRandom.hex}"
       dirname = File.dirname(file_name)
       basename = File.basename(file_name)
-      base_slice = basename.byteslice(0, 254 - tmp_suffix.bytesize)
+      base_slice = byteslice_at_char_boundary(basename, 254 - tmp_suffix.bytesize)
       tmp_path = File.join(dirname, ".#{base_slice}#{tmp_suffix}")
 
       # The temporary name is longer than the final one, so on Windows a
@@ -38,55 +38,72 @@ def self.open(file_name)
         trim = [tmp_suffix.bytesize - (".tmp.".bytesize + 8), overflow].min
         tmp_suffix = tmp_suffix.byteslice(0, tmp_suffix.bytesize - trim)
         overflow -= trim
-        base_slice = base_slice.byteslice(0, [base_slice.bytesize - overflow, 0].max) if overflow > 0
+        base_slice = byteslice_at_char_boundary(base_slice, [base_slice.bytesize - overflow, 0].max) if overflow > 0
         tmp_path = File.join(dirname, ".#{base_slice}#{tmp_suffix}")
       end
 
       flags = File::RDWR | File::CREAT | File::EXCL | File::BINARY
       flags |= File::SHARE_DELETE if defined?(File::SHARE_DELETE)
 
-      File.open(tmp_path, flags) do |temp_file|
+      renamed = false
+      temp_file = File.open(tmp_path, flags)
+
+      begin
         temp_file.binmode
         if old_stat
           # Set correct permissions on new file
           begin
-            File.chown(old_stat.uid, old_stat.gid, temp_file.path)
+            File.chown(old_stat.uid, old_stat.gid, tmp_path)
             # This operation will affect filesystem ACL's
-            File.chmod(old_stat.mode, temp_file.path)
+            File.chmod(old_stat.mode, tmp_path)
           rescue Errno::EPERM, Errno::EACCES
             # Changing file ownership failed, moving on.
           end
         end
 
         return_val = yield temp_file
-      rescue StandardError => error
-        begin
-          temp_file.close
-        rescue StandardError
-          nil
-        end
 
-        begin
-          File.unlink(temp_file.path)
-        rescue StandardError
-          nil
-        end
+        # Any data still buffered is handed to the filesystem on close, so a
+        # failing flush must surface while the destination is still intact.
+        # That means closing the temporary file before the rename. Note this
+        # does not fsync, so the write is atomic but not crash durable.
+        temp_file.close
+        File.rename(tmp_path, file_name)
+        renamed = true
 
-        raise error
-      else
-        begin
-          File.rename(temp_file.path, file_name)
-        rescue StandardError
+        return_val
+      ensure
+        unless renamed
           begin
-            File.unlink(temp_file.path)
+            temp_file.close
           rescue StandardError
+            nil
+          ensure
+            # The unlink runs from an ensure so that a non-StandardError raised by
+            # the close above, a second Ctrl-C for instance, still reaches it. An
+            # interrupt landing on the unlink itself is not covered.
+            begin
+              File.unlink(tmp_path)
+            rescue StandardError
+              nil
+            end
           end
-
-          raise
         end
+      end
+    end
 
-        return_val
+    # Returns the longest prefix of string that is at most max_bytesize bytes
+    # and ends on a character boundary. A string that is invalid in its own
+    # encoding would be cut back to its last valid prefix, discarding an
+    # unbounded part of the name, so it is sliced as raw bytes instead and can
+    # still be cut mid-character.
+    def self.byteslice_at_char_boundary(string, max_bytesize)
+      sliced = string.byteslice(0, max_bytesize)
+      if string.valid_encoding?
+        sliced = sliced.byteslice(0, sliced.bytesize - 1) until sliced.valid_encoding?
       end
+      sliced
     end
+    private_class_method :byteslice_at_char_boundary
   end
 end

test/rubygems/test_gem_util_atomic_file_writer.rb

@@ -4,9 +4,254 @@
 require "rubygems/util/atomic_file_writer"
 
 class TestGemUtilAtomicFileWriter < Gem::TestCase
+  def setup
+    super
+
+    @dir = File.join @tempdir, "atomic"
+    Dir.mkdir @dir
+    @path = File.join @dir, "out.txt"
+  end
+
   def test_external_encoding
-    Gem::AtomicFileWriter.open(File.join(@tempdir, "test.txt")) do |file|
+    Gem::AtomicFileWriter.open(@path) do |file|
       assert_equal(Encoding::ASCII_8BIT, file.external_encoding)
     end
   end
+
+  def test_returns_block_value_and_leaves_no_temp_file
+    result = Gem::AtomicFileWriter.open(@path) do |file|
+      file.write "hello"
+      :done
+    end
+
+    assert_equal :done, result
+    assert_equal "hello", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_content_is_flushed_before_rename
+    original_rename = File.method(:rename)
+    size_at_rename = nil
+
+    rename_spy = lambda do |src, dest|
+      size_at_rename = File.size(src)
+      original_rename.call(src, dest)
+    end
+
+    File.stub(:rename, rename_spy) do
+      Gem::AtomicFileWriter.open(@path) do |file|
+        file.write "hello"
+      end
+    end
+
+    assert_equal 5, size_at_rename
+    assert_equal "hello", File.binread(@path)
+  end
+
+  def test_keeps_destination_and_removes_temp_file_on_error
+    File.binwrite @path, "old"
+
+    error = assert_raise(RuntimeError) do
+      Gem::AtomicFileWriter.open(@path) do |file|
+        file.write "new"
+        raise "boom"
+      end
+    end
+
+    assert_equal "boom", error.message
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_keeps_destination_and_removes_temp_file_on_interrupt
+    File.binwrite @path, "old"
+
+    assert_raise(Interrupt) do
+      Gem::AtomicFileWriter.open(@path) do |file|
+        file.write "new"
+        raise Interrupt
+      end
+    end
+
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_removes_temp_file_when_closing_it_keeps_raising
+    File.binwrite @path, "old"
+
+    temp_file = nil
+    closes = 0
+    raising = true
+    # A real file is closed even when its close raises, so only a stub can keep
+    # raising like this. The writer leaves that file to the garbage collector,
+    # which is why this test closes it itself.
+    failing_open = temp_file_open_stub do |file|
+      temp_file = file
+      file.define_singleton_method(:close) do
+        closes += 1
+        raise Interrupt if raising
+
+        super()
+      end
+    end
+
+    File.stub(:open, failing_open) do
+      assert_raise(Interrupt) do
+        Gem::AtomicFileWriter.open(@path) do |file|
+          file.write "new"
+        end
+      end
+    end
+
+    assert_equal 2, closes
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  ensure
+    raising = false
+    temp_file&.close
+  end
+
+  def test_keeps_destination_when_closing_the_temp_file_fails
+    File.binwrite @path, "old"
+
+    failing_open = temp_file_open_stub do |file|
+      file.define_singleton_method(:close) do
+        super()
+        raise Errno::ENOSPC
+      end
+    end
+
+    File.stub(:open, failing_open) do
+      assert_raise(Errno::ENOSPC) do
+        Gem::AtomicFileWriter.open(@path) do |file|
+          file.write "new"
+        end
+      end
+    end
+
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_keeps_destination_when_renaming_fails
+    File.binwrite @path, "old"
+
+    File.stub(:rename, ->(_src, _dest) { raise Errno::EXDEV }) do
+      assert_raise(Errno::EXDEV) do
+        Gem::AtomicFileWriter.open(@path) do |file|
+          file.write "new"
+        end
+      end
+    end
+
+    assert_equal "old", File.binread(@path)
+    assert_equal ["out.txt"], Dir.children(@dir)
+  end
+
+  def test_preserves_destination_permissions
+    pend "Windows cannot round-trip the POSIX permission bits" if Gem.win_platform?
+
+    File.binwrite @path, "old"
+    File.chmod 0o604, @path
+
+    Gem::AtomicFileWriter.open(@path) do |file|
+      file.write "new"
+    end
+
+    assert_equal "new", File.binread(@path)
+    assert_equal 0o604, File.stat(@path).mode & 0o777
+  end
+
+  def test_multibyte_basename_is_truncated_at_char_boundary
+    pend "long file names easily exceed MAX_PATH on Windows" if Gem.win_platform?
+
+    path = File.join @dir, "#{"あ" * 82}.txt"
+    original_rename = File.method(:rename)
+    tmp_basename = nil
+
+    rename_spy = lambda do |src, dest|
+      tmp_basename ||= File.basename(src)
+      original_rename.call(src, dest)
+    end
+
+    # Filesystems that store names as raw bytes, such as ext4, happily create a
+    # temporary file whose name is cut in the middle of a character, so assert on
+    # the name itself rather than on the destination write failing.
+    File.stub(:rename, rename_spy) do
+      Gem::AtomicFileWriter.open(path) do |file|
+        file.write "hello"
+      end
+    end
+
+    assert_predicate tmp_basename, :valid_encoding?
+    assert_operator tmp_basename.bytesize, :<=, 255
+    assert_equal "hello", File.binread(path)
+    assert_equal 1, Dir.children(@dir).size
+  end
+
+  def test_long_basename_is_truncated_to_the_name_length_limit
+    pend "long file names easily exceed MAX_PATH on Windows" if Gem.win_platform?
+
+    path = File.join @dir, "#{"a" * 250}.txt"
+    original_rename = File.method(:rename)
+    tmp_basename = nil
+
+    rename_spy = lambda do |src, dest|
+      tmp_basename ||= File.basename(src)
+      original_rename.call(src, dest)
+    end
+
+    File.stub(:rename, rename_spy) do
+      Gem::AtomicFileWriter.open(path) do |file|
+        file.write "hello"
+      end
+    end
+
+    assert_equal 255, tmp_basename.bytesize
+    assert_equal "hello", File.binread(path)
+  end
+
+  def test_byteslice_at_char_boundary
+    sliced = Gem::AtomicFileWriter.send :byteslice_at_char_boundary, "あいう", 4
+
+    assert_equal "あ", sliced
+    assert_predicate sliced, :valid_encoding?
+  end
+
+  def test_byteslice_at_char_boundary_with_invalid_encoding
+    invalid = "\xFFabc".dup.force_encoding(Encoding::UTF_8)
+    refute_predicate invalid, :valid_encoding?
+
+    sliced = Gem::AtomicFileWriter.send :byteslice_at_char_boundary, invalid, 2
+
+    assert_equal "\xFFa".dup.force_encoding(Encoding::UTF_8), sliced
+  end
+
+  private
+
+  # Returns a File.open replacement that hands the writer's temporary file to
+  # the given block, which installs whatever close behaviour a test needs.
+  # Every other path is opened normally. Call this before installing the stub,
+  # since it captures the current File.open.
+  def temp_file_open_stub(&injection)
+    original_open = File.method(:open)
+    prefix = ".#{File.basename(@path)}.tmp."
+
+    lambda do |name, *args, **kwargs, &block|
+      unless File.basename(name.to_s).start_with?(prefix)
+        next original_open.call(name, *args, **kwargs, &block)
+      end
+      raise ArgumentError, "this stub cannot replace close on a File.open with a block" if block
+
+      file = original_open.call(name, *args, **kwargs)
+      begin
+        injection.call(file)
+      rescue StandardError
+        file.close
+        raise
+      end
+      file
+    end
+  end
 end