Write gem files atomically

This change updates `write_binary` to use a new class,
`AtomicFile.write` 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.

Author: eileencodes

Manifest.txt

@@ -529,6 +529,7 @@ lib/rubygems/uri.rb
 lib/rubygems/uri_formatter.rb
 lib/rubygems/user_interaction.rb
 lib/rubygems/util.rb
+lib/rubygems/util/atomic_file.rb
 lib/rubygems/util/licenses.rb
 lib/rubygems/util/list.rb
 lib/rubygems/validator.rb

lib/rubygems.rb

@@ -17,6 +17,7 @@ module Gem
 require_relative "rubygems/errors"
 require_relative "rubygems/target_rbconfig"
 require_relative "rubygems/win_platform"
+require_relative "rubygems/util/atomic_file"
 
 ##
 # RubyGems is the Ruby standard for publishing and managing third party
@@ -833,14 +834,12 @@ def self.read_binary(path)
   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::AtomicFile.write(path) do |file|
+      file.write(data)
+    end
   end
 
   ##

lib/rubygems/util/atomic_file.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+module Gem
+  class AtomicFile
+    ##
+    # Write to a file atomically. Useful for situations where you don't
+    # want other processes or threads to see half-written files.
+
+    def self.write(file_name)
+      temp_dir = File.dirname(file_name)
+      require "tempfile" unless defined?(Tempfile)
+
+      Tempfile.open(".#{File.basename(file_name)}", temp_dir) do |temp_file|
+        temp_file.binmode
+        return_file = 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_file
+      end
+    end
+
+    def self.probe_permissions_in(dir) # :nodoc:
+      require "fileutils" unless defined?(FileUtils)
+
+      basename = [
+        ".permissions_check",
+        Thread.current.object_id,
+        Process.pid,
+        rand(1000000)
+      ].join(".")
+
+      file_name = File.join(dir, basename)
+      FileUtils.touch(file_name)
+      File.stat(file_name)
+    rescue Errno::ENOENT
+      file_name = nil
+    ensure
+      FileUtils.rm_f(file_name) if file_name
+    end
+  end
+end

test/rubygems/test_gem_installer.rb

@@ -2385,25 +2385,31 @@ def test_leaves_no_empty_cached_spec_when_no_more_disk_space
     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::AtomicFile.write(@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 @@ def test_cache_update_path
     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 @@ def test_cache_update_path_with_utf8_internal_encoding
       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 @@ def test_cache_update_path_no_update
     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 @@ def test_cache_update_path_no_update
     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
-
-    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
+  def test_cache_update_path_overwrites_existing_file
+    uri = Gem::URI "http://example/file"
+    path = File.join @tempdir, "file"
 
-        raise Gem::RemoteFetcher::FetchError.new("haha!", "")
-      end
-    else
-      def fetcher.fetch_path(arg, *rest)
-        @test_arg = arg
-        @test_data
-      end
-    end
+    # Create existing file with old content
+    File.write(path, "old content")
+    assert_equal "old content", File.read(path)
 
-    fetcher
+    fetcher = fake_fetcher(uri.to_s, "new content")
+
+    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:[email protected]/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:[email protected]")
-    assert_equal("http://user:[email protected]/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://[email protected]/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://[email protected]")
-    assert_equal("http://[email protected]/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:[email protected]/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:[email protected]")
-    assert_equal("http://token:[email protected]/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%[email protected]/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%[email protected]")
-    assert_equal("http://user:%25pas%[email protected]/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 @@ def test_download_local_space
 
   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 @@ def test_download_install_dir
     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 @@ def test_download_read_only
       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 @@ def test_download_platform_legacy
     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 @@ def test_yaml_error_on_size
     end
   end
 
+  private
+
   def assert_error(exception_class = Exception)
     got_exception = false
 
@@ -603,4 +588,10 @@ def assert_error(exception_class = Exception)
 
     assert got_exception, "Expected exception conforming to #{exception_class}"
   end
+
+  def fake_fetcher(url, data)
+    fetcher = Gem::FakeFetcher.new
+    fetcher.data[url] = data
+    Gem::RemoteFetcher.fetcher = fetcher
+  end
 end