From bdbe8d50150447748eaa92a0cce7327d8dec9903 Mon Sep 17 00:00:00 2001 From: eileencodes Date: Tue, 25 Nov 2025 14:17:03 -0500 Subject: [PATCH] [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 | 3 +- lib/rubygems.rb | 11 +- lib/rubygems/util/atomic_file_writer.rb | 67 ++++++++++++ test/rubygems/test_gem_installer.rb | 34 +++--- test/rubygems/test_gem_remote_fetcher.rb | 132 +++++++++++------------ 5 files changed, 157 insertions(+), 90 deletions(-) create mode 100644 lib/rubygems/util/atomic_file_writer.rb diff --git a/lib/bundler/shared_helpers.rb b/lib/bundler/shared_helpers.rb index 6419e42997..2aa8abe0a0 100644 --- a/lib/bundler/shared_helpers.rb +++ b/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 diff --git a/lib/rubygems.rb b/lib/rubygems.rb index e99176fec0..41b39a808b 100644 --- a/lib/rubygems.rb +++ b/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 ## diff --git a/lib/rubygems/util/atomic_file_writer.rb b/lib/rubygems/util/atomic_file_writer.rb new file mode 100644 index 0000000000..7d1d6a7416 --- /dev/null +++ b/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 diff --git a/test/rubygems/test_gem_installer.rb b/test/rubygems/test_gem_installer.rb index 293fe1e823..0220a41f88 100644 --- a/test/rubygems/test_gem_installer.rb +++ b/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 diff --git a/test/rubygems/test_gem_remote_fetcher.rb b/test/rubygems/test_gem_remote_fetcher.rb index 5c1d89fad6..9badd75b42 100644 --- a/test/rubygems/test_gem_remote_fetcher.rb +++ b/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