Skip to content

Commit f46d937

Browse files
committed
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.
1 parent 56c8d13 commit f46d937

File tree

6 files changed

+159
-91
lines changed

6 files changed

+159
-91
lines changed

Manifest.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,7 @@ lib/rubygems/uri.rb
528528
lib/rubygems/uri_formatter.rb
529529
lib/rubygems/user_interaction.rb
530530
lib/rubygems/util.rb
531+
lib/rubygems/util/atomic_file.rb
531532
lib/rubygems/util/licenses.rb
532533
lib/rubygems/util/list.rb
533534
lib/rubygems/validator.rb

bundler/lib/bundler/shared_helpers.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,8 @@ def set_bundle_environment
105105
def filesystem_access(path, action = :write, &block)
106106
yield(path.dup)
107107
rescue Errno::EACCES => e
108-
raise unless e.message.include?(path.to_s) || action == :create
108+
path_basename = File.basename(path.to_s)
109+
raise unless e.message.include?(path_basename) || action == :create
109110

110111
raise PermissionError.new(path, action)
111112
rescue Errno::EAGAIN

lib/rubygems.rb

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ module Gem
1717
require_relative "rubygems/errors"
1818
require_relative "rubygems/target_rbconfig"
1919
require_relative "rubygems/win_platform"
20+
require_relative "rubygems/util/atomic_file_writer"
2021

2122
##
2223
# RubyGems is the Ruby standard for publishing and managing third party
@@ -833,14 +834,12 @@ def self.read_binary(path)
833834
end
834835

835836
##
836-
# Safely write a file in binary mode on all platforms.
837+
# Atomically write a file in binary mode on all platforms.
837838

838839
def self.write_binary(path, data)
839-
File.binwrite(path, data)
840-
rescue Errno::ENOSPC
841-
# If we ran out of space but the file exists, it's *guaranteed* to be corrupted.
842-
File.delete(path) if File.exist?(path)
843-
raise
840+
Gem::AtomicFileWriter.open(path) do |file|
841+
file.write(data)
842+
end
844843
end
845844

846845
##
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
# frozen_string_literal: true
2+
3+
# Based on ActiveSupport's AtomicFile implementation
4+
# Copyright (c) David Heinemeier Hansson
5+
# https://github.com/rails/rails/blob/main/activesupport/lib/active_support/atomic_file.rb
6+
# Licensed under the MIT License
7+
8+
module Gem
9+
class AtomicFileWriter
10+
##
11+
# Write to a file atomically. Useful for situations where you don't
12+
# want other processes or threads to see half-written files.
13+
14+
def self.open(file_name)
15+
temp_dir = File.dirname(file_name)
16+
require "tempfile" unless defined?(Tempfile)
17+
18+
Tempfile.open(".#{File.basename(file_name)}", temp_dir) do |temp_file|
19+
temp_file.binmode
20+
return_value = yield temp_file
21+
temp_file.close
22+
23+
original_permissions = if File.exist?(file_name)
24+
File.stat(file_name)
25+
else
26+
# If not possible, probe which are the default permissions in the
27+
# destination directory.
28+
probe_permissions_in(File.dirname(file_name))
29+
end
30+
31+
# Set correct permissions on new file
32+
if original_permissions
33+
begin
34+
File.chown(original_permissions.uid, original_permissions.gid, temp_file.path)
35+
File.chmod(original_permissions.mode, temp_file.path)
36+
rescue Errno::EPERM, Errno::EACCES
37+
# Changing file ownership failed, moving on.
38+
end
39+
end
40+
41+
# Overwrite original file with temp file
42+
File.rename(temp_file.path, file_name)
43+
return_value
44+
end
45+
end
46+
47+
def self.probe_permissions_in(dir) # :nodoc:
48+
basename = [
49+
".permissions_check",
50+
Thread.current.object_id,
51+
Process.pid,
52+
rand(1_000_000),
53+
].join(".")
54+
55+
file_name = File.join(dir, basename)
56+
File.open(file_name, "w") {}
57+
File.stat(file_name)
58+
rescue Errno::ENOENT
59+
nil
60+
ensure
61+
begin
62+
File.unlink(file_name) if File.exist?(file_name)
63+
rescue SystemCallError
64+
end
65+
end
66+
end
67+
end

test/rubygems/test_gem_installer.rb

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2385,25 +2385,31 @@ def test_leaves_no_empty_cached_spec_when_no_more_disk_space
23852385
installer = Gem::Installer.for_spec @spec
23862386
installer.gem_home = @gemhome
23872387

2388-
File.singleton_class.class_eval do
2389-
alias_method :original_binwrite, :binwrite
2390-
2391-
def binwrite(path, data)
2388+
assert_raise(Errno::ENOSPC) do
2389+
Gem::AtomicFileWriter.open(@spec.spec_file) do
23922390
raise Errno::ENOSPC
23932391
end
23942392
end
23952393

2396-
assert_raise Errno::ENOSPC do
2397-
installer.write_spec
2398-
end
2399-
24002394
assert_path_not_exist @spec.spec_file
2401-
ensure
2402-
File.singleton_class.class_eval do
2403-
remove_method :binwrite
2404-
alias_method :binwrite, :original_binwrite
2405-
remove_method :original_binwrite
2406-
end
2395+
end
2396+
2397+
def test_write_default_spec
2398+
@spec = setup_base_spec
2399+
@spec.files = %w[a.rb b.rb c.rb]
2400+
2401+
installer = Gem::Installer.for_spec @spec
2402+
installer.gem_home = @gemhome
2403+
2404+
installer.write_default_spec
2405+
2406+
assert_path_exist installer.default_spec_file
2407+
2408+
loaded = Gem::Specification.load installer.default_spec_file
2409+
2410+
assert_equal @spec.files, loaded.files
2411+
assert_equal @spec.name, loaded.name
2412+
assert_equal @spec.version, loaded.version
24072413
end
24082414

24092415
def test_dir

0 commit comments

Comments
 (0)