[ruby/rubygems] Write gem files atomically

This change updates `write_binary` to use a new class,
`AtomicFileWriter.open` to write the gem's files. This implementation
is borrowed from Active Support's [`atomic_write`](https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/file/atomic.rb).

Atomic write will write the files to a temporary file and then once
created, sets permissions and renames the file. If the file is corrupted
- ie on failed download, an error occurs, or for some other reason, the
real file will not be created. The changes made here make `verify_gz`
obsolete, we don't need to verify it if we have successfully created the
file atomically. If it exists, it is not corrupt. If it is corrupt, the
file won't exist on disk.

While writing tests for this functionality I replaced the
`RemoteFetcher` stub with `FakeFetcher` except for where we really do
need to overwrite the `RemoteFetcher`. The new test implementation is much
clearer on what it's trying to accomplish versus the prior test
implementation.

https://github.com/ruby/rubygems/commit/0cd4b54291

lib/bundler/shared_helpers.rb

@@ -105,7 +105,8 @@ module Bundler
     def filesystem_access(path, action = :write, &block)
       yield(path.dup)
     rescue Errno::EACCES => e
-      raise unless e.message.include?(path.to_s) || action == :create
+      path_basename = File.basename(path.to_s)
+      raise unless e.message.include?(path_basename) || action == :create
 
       raise PermissionError.new(path, action)
     rescue Errno::EAGAIN

lib/rubygems.rb

@@ -17,6 +17,7 @@ require_relative "rubygems/deprecate"
 require_relative "rubygems/errors"
 require_relative "rubygems/target_rbconfig"
 require_relative "rubygems/win_platform"
+require_relative "rubygems/util/atomic_file_writer"
 
 ##
 # RubyGems is the Ruby standard for publishing and managing third party
@@ -833,14 +834,12 @@ An Array (#{env.inspect}) was passed in from #{caller[3]}
   end
 
   ##
-  # Safely write a file in binary mode on all platforms.
+  # Atomically write a file in binary mode on all platforms.
 
   def self.write_binary(path, data)
-    File.binwrite(path, data)
-  rescue Errno::ENOSPC
-    # If we ran out of space but the file exists, it's *guaranteed* to be corrupted.
-    File.delete(path) if File.exist?(path)
-    raise
+    Gem::AtomicFileWriter.open(path) do |file|
+      file.write(data)
+    end
   end
 
   ##

lib/rubygems/util/atomic_file_writer.rb

@@ -0,0 +1,67 @@
+# frozen_string_literal: true
+
+# Based on ActiveSupport's AtomicFile implementation
+# Copyright (c) David Heinemeier Hansson
+# https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/file/atomic.rb
+# Licensed under the MIT License
+
+module Gem
+  class AtomicFileWriter
+    ##
+    # Write to a file atomically. Useful for situations where you don't
+    # want other processes or threads to see half-written files.
+
+    def self.open(file_name)
+      temp_dir = File.dirname(file_name)
+      require "tempfile" unless defined?(Tempfile)
+
+      Tempfile.create(".#{File.basename(file_name)}", temp_dir) do |temp_file|
+        temp_file.binmode
+        return_value = yield temp_file
+        temp_file.close
+
+        original_permissions = if File.exist?(file_name)
+          File.stat(file_name)
+        else
+          # If not possible, probe which are the default permissions in the
+          # destination directory.
+          probe_permissions_in(File.dirname(file_name))
+        end
+
+        # Set correct permissions on new file
+        if original_permissions
+          begin
+            File.chown(original_permissions.uid, original_permissions.gid, temp_file.path)
+            File.chmod(original_permissions.mode, temp_file.path)
+          rescue Errno::EPERM, Errno::EACCES
+            # Changing file ownership failed, moving on.
+          end
+        end
+
+        # Overwrite original file with temp file
+        File.rename(temp_file.path, file_name)
+        return_value
+      end
+    end
+
+    def self.probe_permissions_in(dir) # :nodoc:
+      basename = [
+        ".permissions_check",
+        Thread.current.object_id,
+        Process.pid,
+        rand(1_000_000),
+      ].join(".")
+
+      file_name = File.join(dir, basename)
+      File.open(file_name, "w") {}
+      File.stat(file_name)
+    rescue Errno::ENOENT
+      nil
+    ensure
+      begin
+        File.unlink(file_name) if File.exist?(file_name)
+      rescue SystemCallError
+      end
+    end
+  end
+end

test/rubygems/test_gem_installer.rb

@@ -2385,25 +2385,31 @@ class TestGemInstaller < Gem::InstallerTestCase
     installer = Gem::Installer.for_spec @spec
     installer.gem_home = @gemhome
 
-    File.singleton_class.class_eval do
-      alias_method :original_binwrite, :binwrite
-
-      def binwrite(path, data)
+    assert_raise(Errno::ENOSPC) do
+      Gem::AtomicFileWriter.open(@spec.spec_file) do
         raise Errno::ENOSPC
       end
     end
 
-    assert_raise Errno::ENOSPC do
-      installer.write_spec
-    end
-
     assert_path_not_exist @spec.spec_file
-  ensure
-    File.singleton_class.class_eval do
-      remove_method :binwrite
-      alias_method :binwrite, :original_binwrite
-      remove_method :original_binwrite
-    end
+  end
+
+  def test_write_default_spec
+    @spec = setup_base_spec
+    @spec.files = %w[a.rb b.rb c.rb]
+
+    installer = Gem::Installer.for_spec @spec
+    installer.gem_home = @gemhome
+
+    installer.write_default_spec
+
+    assert_path_exist installer.default_spec_file
+
+    loaded = Gem::Specification.load installer.default_spec_file
+
+    assert_equal @spec.files, loaded.files
+    assert_equal @spec.name, loaded.name
+    assert_equal @spec.version, loaded.version
   end
 
   def test_dir

test/rubygems/test_gem_remote_fetcher.rb

@@ -60,7 +60,7 @@ class TestGemRemoteFetcher < Gem::TestCase
     uri = Gem::URI "http://example/file"
     path = File.join @tempdir, "file"
 
-    fetcher = util_fuck_with_fetcher "hello"
+    fetcher = fake_fetcher(uri.to_s, "hello")
 
     data = fetcher.cache_update_path uri, path
 
@@ -75,7 +75,7 @@ class TestGemRemoteFetcher < Gem::TestCase
       path = File.join @tempdir, "file"
       data = String.new("\xC8").force_encoding(Encoding::BINARY)
 
-      fetcher = util_fuck_with_fetcher data
+      fetcher = fake_fetcher(uri.to_s, data)
 
       written_data = fetcher.cache_update_path uri, path
 
@@ -88,7 +88,7 @@ class TestGemRemoteFetcher < Gem::TestCase
     uri = Gem::URI "http://example/file"
     path = File.join @tempdir, "file"
 
-    fetcher = util_fuck_with_fetcher "hello"
+    fetcher = fake_fetcher(uri.to_s, "hello")
 
     data = fetcher.cache_update_path uri, path, false
 
@@ -97,103 +97,79 @@ class TestGemRemoteFetcher < Gem::TestCase
     assert_path_not_exist path
   end
 
-  def util_fuck_with_fetcher(data, blow = false)
-    fetcher = Gem::RemoteFetcher.fetcher
-    fetcher.instance_variable_set :@test_data, data
+  def test_cache_update_path_overwrites_existing_file
+    uri = Gem::URI "http://example/file"
+    path = File.join @tempdir, "file"
 
-    if blow
-      def fetcher.fetch_path(arg, *rest)
-        # OMG I'm such an ass
-        class << self; remove_method :fetch_path; end
-        def self.fetch_path(arg, *rest)
-          @test_arg = arg
-          @test_data
-        end
+    # Create existing file with old content
+    File.write(path, "old content")
+    assert_equal "old content", File.read(path)
 
-        raise Gem::RemoteFetcher::FetchError.new("haha!", "")
-      end
-    else
-      def fetcher.fetch_path(arg, *rest)
-        @test_arg = arg
-        @test_data
-      end
-    end
+    fetcher = fake_fetcher(uri.to_s, "new content")
 
-    fetcher
+    data = fetcher.cache_update_path uri, path
+
+    assert_equal "new content", data
+    assert_equal "new content", File.read(path)
   end
 
   def test_download
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://gems.example.com")
-    assert_equal("http://gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_auth
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://user:password@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://user:password@gems.example.com")
-    assert_equal("http://user:password@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_token
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://token@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://token@gems.example.com")
-    assert_equal("http://token@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_x_oauth_basic
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://token:x-oauth-basic@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://token:x-oauth-basic@gems.example.com")
-    assert_equal("http://token:x-oauth-basic@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
   def test_download_with_encoded_auth
-    a1_data = nil
-    File.open @a1_gem, "rb" do |fp|
-      a1_data = fp.read
-    end
+    a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://user:%25pas%25sword@gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     a1_cache_gem = @a1.cache_file
     assert_equal a1_cache_gem, fetcher.download(@a1, "http://user:%25pas%25sword@gems.example.com")
-    assert_equal("http://user:%25pas%25sword@gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
     assert File.exist?(a1_cache_gem)
   end
 
@@ -235,8 +211,9 @@ class TestGemRemoteFetcher < Gem::TestCase
 
   def test_download_install_dir
     a1_data = File.open @a1_gem, "rb", &:read
+    a1_url = "http://gems.example.com/gems/a-1.gem"
 
-    fetcher = util_fuck_with_fetcher a1_data
+    fetcher = fake_fetcher(a1_url, a1_data)
 
     install_dir = File.join @tempdir, "more_gems"
 
@@ -245,8 +222,7 @@ class TestGemRemoteFetcher < Gem::TestCase
     actual = fetcher.download(@a1, "http://gems.example.com", install_dir)
 
     assert_equal a1_cache_gem, actual
-    assert_equal("http://gems.example.com/gems/a-1.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
+    assert_equal a1_url, fetcher.paths.last
 
     assert File.exist?(a1_cache_gem)
   end
@@ -282,7 +258,12 @@ class TestGemRemoteFetcher < Gem::TestCase
       FileUtils.chmod 0o555, @a1.cache_dir
       FileUtils.chmod 0o555, @gemhome
 
-      fetcher = util_fuck_with_fetcher File.read(@a1_gem)
+      fetcher = Gem::RemoteFetcher.fetcher
+      def fetcher.fetch_path(uri, *rest)
+        File.read File.join(@test_gem_dir, "a-1.gem")
+      end
+      fetcher.instance_variable_set(:@test_gem_dir, File.dirname(@a1_gem))
+
       fetcher.download(@a1, "http://gems.example.com")
       a1_cache_gem = File.join Gem.user_dir, "cache", @a1.file_name
       assert File.exist? a1_cache_gem
@@ -301,19 +282,21 @@ class TestGemRemoteFetcher < Gem::TestCase
     end
     e1.loaded_from = File.join(@gemhome, "specifications", e1.full_name)
 
-    e1_data = nil
-    File.open e1_gem, "rb" do |fp|
-      e1_data = fp.read
-    end
+    e1_data = File.open e1_gem, "rb", &:read
 
-    fetcher = util_fuck_with_fetcher e1_data, :blow_chunks
+    fetcher = Gem::RemoteFetcher.fetcher
+    def fetcher.fetch_path(uri, *rest)
+      @call_count ||= 0
+      @call_count += 1
+      raise Gem::RemoteFetcher::FetchError.new("error", uri) if @call_count == 1
+      @test_data
+    end
+    fetcher.instance_variable_set(:@test_data, e1_data)
 
     e1_cache_gem = e1.cache_file
 
     assert_equal e1_cache_gem, fetcher.download(e1, "http://gems.example.com")
 
-    assert_equal("http://gems.example.com/gems/#{e1.original_name}.gem",
-                 fetcher.instance_variable_get(:@test_arg).to_s)
     assert File.exist?(e1_cache_gem)
   end
 
@@ -592,6 +575,8 @@ class TestGemRemoteFetcher < Gem::TestCase
     end
   end
 
+  private
+
   def assert_error(exception_class = Exception)
     got_exception = false
 
@@ -603,4 +588,13 @@ class TestGemRemoteFetcher < Gem::TestCase
 
     assert got_exception, "Expected exception conforming to #{exception_class}"
   end
+
+  def fake_fetcher(url, data)
+    original_fetcher = Gem::RemoteFetcher.fetcher
+    fetcher = Gem::FakeFetcher.new
+    fetcher.data[url] = data
+    Gem::RemoteFetcher.fetcher = fetcher
+  ensure
+    Gem::RemoteFetcher.fetcher = original_fetcher
+  end
 end