[rubygems/rubygems] Properly protect writing binstubs with a file lock

There's an issue when multiple processes try to write the same binstub.
The problem is that our file locking mechanism is incorrect because
files are truncated _before_ they are locked. So it can happen that:

* Process A truncates binstub FOO.
* Process B truncates binstub FOO.
* Process A writes binstub FOO for gem BAR from the beginning of file.
* Process B writes binstub FOO for gem BAZ from the beginning of file.

If binstub FOO for gem BAR is bigger than binstub FOO for gem BAZ, then
some bytes will be left around at the end of the binstub, making it
corrupt.

This was not a problem in our specs until the spec testing binstubs with
the same name coming from different gems changed from using gems named
"fake" and "rack" to using gems named "fake" and "myrack". Because of
the difference in gem name length, the generated binstub for gem
"myrack" is now longer, causing the above problem if binstub for gem
myrack is written first.

The solution is to make sure when using flock to always use modes that
DON'T truncate the file when opening it. So, we use `r+` if the file
exists previously (it requires the file to exist previously), otherwise
we use `a+`.

https://github.com/rubygems/rubygems/commit/ce8bcba90f

2 files changed, 9 insertions, 16 deletions

diff --git a/lib/rubygems.rb b/lib/rubygems.rb
index e35d05af9b..2b52cde0a7 100644
--- a/lib/rubygems.rb
+++ b/lib/rubygems.rb
@@ -792,21 +792,18 @@ An Array (#{env.inspect}) was passed in from #{caller[3]}
   end
 
   ##
-  # Open a file with given flags. It requires special logic on Windows, like
-  # protecting access with flock
+  # Open a file with given flags
 
   def self.open_file(path, flags, &block)
-    if !java_platform? && win_platform?
-      open_file_with_flock(path, flags, &block)
-    else
-      open_file_without_flock(path, flags, &block)
-    end
+    File.open(path, flags, &block)
   end
 
   ##
   # Open a file with given flags, and protect access with flock
 
-  def self.open_file_with_flock(path, flags, &block)
+  def self.open_file_with_flock(path, &block)
+    flags = File.exist?(path) ? "r+" : "a+"
+
     File.open(path, flags) do |io|
       begin
         io.flock(File::LOCK_EX)
@@ -817,7 +814,7 @@ An Array (#{env.inspect}) was passed in from #{caller[3]}
       if Thread.main != Thread.current
         raise
       else
-        open_file_without_flock(path, flags, &block)
+        open_file(path, flags, &block)
       end
     end
   end
@@ -1318,10 +1315,6 @@ An Array (#{env.inspect}) was passed in from #{caller[3]}
 
     private
 
-    def open_file_without_flock(path, flags, &block)
-      File.open(path, flags, &block)
-    end
-
     def already_loaded?(file)
       $LOADED_FEATURES.any? do |feature_path|
         feature_path.end_with?(file) && default_gem_load_paths.any? {|load_path_entry| feature_path == "#{load_path_entry}/#{file}" }
diff --git a/lib/rubygems/installer.rb b/lib/rubygems/installer.rb
index f637e7b604..3705174ff0 100644
--- a/lib/rubygems/installer.rb
+++ b/lib/rubygems/installer.rb
@@ -222,7 +222,7 @@ class Gem::Installer
     ruby_executable = false
     existing = nil
 
-    Gem.open_file_with_flock generated_bin, "rb+" do |io|
+    File.open generated_bin, "rb" do |io|
       line = io.gets
       shebang = /^#!.*ruby/o
 
@@ -541,8 +541,8 @@ class Gem::Installer
     require "fileutils"
     FileUtils.rm_f bin_script_path # prior install may have been --no-wrappers
 
-    File.open bin_script_path, "wb", 0o755 do |file|
-      file.print app_script_text(filename)
+    Gem.open_file_with_flock(bin_script_path) do |file|
+      file.write app_script_text(filename)
       file.chmod(options[:prog_mode] || 0o755)
     end