[ruby/rubygems] Make install_location private and hoist fn expansion

`install_location` is a hot path during gem installation.  I want to
make the method private so that we can freely refactor it. This commit
makes it private and ports the tests to use integration tests instead of
directly calling the method.

Before this commit, `install_location` would repeatedly call
`File.realpath` on the destination directory when `extract_tar_gz` had
already done that work.

ruby/rubygems@c15a559007

Author: tenderlove

lib/rubygems/package.rb

@@ -510,24 +510,6 @@ def gzip_to(io) # :yields: gz_io
     gz_io.close
   end
 
-  ##
-  # Returns the full path for installing +filename+.
-  #
-  # If +filename+ is not inside +destination_dir+ an exception is raised.
-
-  def install_location(filename, destination_dir) # :nodoc:
-    raise Gem::Package::PathError.new(filename, destination_dir) if
-      filename.start_with? "/"
-
-    destination_dir = File.realpath(destination_dir)
-    destination = File.expand_path(filename, destination_dir)
-
-    raise Gem::Package::PathError.new(destination, destination_dir) unless
-      normalize_path(destination).start_with? normalize_path(destination_dir + "/")
-
-    destination
-  end
-
   if Gem.win_platform?
     def normalize_path(pathname) # :nodoc:
       pathname.downcase
@@ -662,6 +644,24 @@ def verify
 
   private
 
+  ##
+  # Returns the full path for installing +filename+ into +destination_dir+,
+  # which must already be resolved with File.realpath by the caller.
+  #
+  # If +filename+ is not inside +destination_dir+ an exception is raised.
+
+  def install_location(filename, destination_dir) # :nodoc:
+    raise Gem::Package::PathError.new(filename, destination_dir) if
+      filename.start_with? "/"
+
+    destination = File.expand_path(filename, destination_dir)
+
+    raise Gem::Package::PathError.new(destination, destination_dir) unless
+      normalize_path(destination).start_with? normalize_path(destination_dir + "/")
+
+    destination
+  end
+
   ##
   # Verifies the +checksums+ against the +digests+.  This check is not
   # cryptographically secure.  Missing checksums are ignored.

lib/rubygems/package/old.rb

@@ -59,6 +59,9 @@ def extract_files(destination_dir)
       header = file_list io
       raise Gem::Exception, errstr unless header
 
+      # install_location expects an already-resolved destination dir
+      destination_dir = File.realpath(destination_dir)
+
       header.each do |entry|
         full_name = entry["path"]
 

test/rubygems/test_gem_package.rb

@@ -835,79 +835,100 @@ def test_extract_tar_gz_case_insensitive
     end
   end
 
-  def test_install_location
-    package = Gem::Package.new @gem
-
-    file = "file.rb".dup
-
-    destination = package.install_location file, @destination
-
-    assert_equal File.join(@destination, "file.rb"), destination
-  end
+  # The following tests exercise install_location's path resolution and
+  # traversal protection through the real extraction path (extract_tar_gz)
+  # rather than calling the private helper directly. The absolute-path case is
+  # already covered by test_extract_tar_gz_absolute.
 
-  def test_install_location_absolute
+  def test_extract_tar_gz_basic_file
     package = Gem::Package.new @gem
 
-    e = assert_raise Gem::Package::PathError do
-      package.install_location "/absolute.rb", @destination
+    tgz_io = util_tar_gz do |tar|
+      tar.add_file "file.rb", 0o644 do |io|
+        io.write "hi"
+      end
     end
 
-    assert_equal("installing into parent path /absolute.rb of " \
-                 "#{@destination} is not allowed", e.message)
+    package.extract_tar_gz tgz_io, @destination
+
+    extracted = File.join @destination, "file.rb"
+    assert_path_exist extracted
+    assert_equal "hi", File.read(extracted)
   end
 
-  def test_install_location_dots
+  def test_extract_tar_gz_collapses_parent_dots
     package = Gem::Package.new @gem
 
-    file = "file.rb"
-
-    destination = File.join @destination, "foo", "..", "bar"
-
-    FileUtils.mkdir_p File.join @destination, "foo"
-    FileUtils.mkdir_p File.expand_path destination
+    tgz_io = util_tar_gz do |tar|
+      tar.add_file "foo/../bar/file.rb", 0o644 do |io|
+        io.write "hi"
+      end
+    end
 
-    destination = package.install_location file, destination
+    package.extract_tar_gz tgz_io, @destination
 
-    # this test only fails on ruby missing File.realpath
-    assert_equal File.join(@destination, "bar", "file.rb"), destination
+    extracted = File.join @destination, "bar", "file.rb"
+    assert_path_exist extracted
+    assert_equal "hi", File.read(extracted)
+    assert_path_not_exist File.join(@destination, "foo")
   end
 
-  def test_install_location_extra_slash
+  def test_extract_tar_gz_collapses_extra_slash
     package = Gem::Package.new @gem
 
-    file = "foo//file.rb".dup
+    tgz_io = util_tar_gz do |tar|
+      tar.add_file "foo//file.rb", 0o644 do |io|
+        io.write "hi"
+      end
+    end
 
-    destination = package.install_location file, @destination
+    package.extract_tar_gz tgz_io, @destination
 
-    assert_equal File.join(@destination, "foo", "file.rb"), destination
+    extracted = File.join @destination, "foo", "file.rb"
+    assert_path_exist extracted
+    assert_equal "hi", File.read(extracted)
   end
 
-  def test_install_location_relative
+  def test_extract_tar_gz_rejects_relative_escape
     package = Gem::Package.new @gem
 
+    tgz_io = util_tar_gz do |tar|
+      tar.add_file "../relative.rb", 0o644 do |io|
+        io.write "hi"
+      end
+    end
+
     e = assert_raise Gem::Package::PathError do
-      package.install_location "../relative.rb", @destination
+      package.extract_tar_gz tgz_io, @destination
     end
 
     parent = File.expand_path File.join @destination, "../relative.rb"
 
     assert_equal("installing into parent path #{parent} of " \
                  "#{@destination} is not allowed", e.message)
+    assert_path_not_exist parent
   end
 
-  def test_install_location_suffix
+  def test_extract_tar_gz_rejects_suffix_escape
     package = Gem::Package.new @gem
 
     filename = "../#{File.basename(@destination)}suffix.rb"
 
+    tgz_io = util_tar_gz do |tar|
+      tar.add_file filename, 0o644 do |io|
+        io.write "hi"
+      end
+    end
+
     e = assert_raise Gem::Package::PathError do
-      package.install_location filename, @destination
+      package.extract_tar_gz tgz_io, @destination
     end
 
     parent = File.expand_path File.join @destination, filename
 
     assert_equal("installing into parent path #{parent} of " \
                  "#{@destination} is not allowed", e.message)
+    assert_path_not_exist parent
   end
 
   def test_load_spec_from_metadata