* lib/rubygems: Fix CVE-2013-4363. Miscellaneous minor improvements.

* test/rubygems:  Tests for the above.


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@43039 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

ChangeLog

@@ -1,3 +1,9 @@
+Wed Sep 25 09:53:11 2013  Eric Hodel  <drbrain@segment7.net>
+
+	* lib/rubygems:  Fix CVE-2013-4363.  Miscellaneous minor improvements.
+
+	* test/rubygems:  Tests for the above.
+
 Tue Sep 24 17:38:56 2013  Nobuyoshi Nakada  <nobu@ruby-lang.org>
 
 	* string.c (rb_str_inspect): get rid of out-of-bound access.

lib/rubygems/commands/specification_command.rb

@@ -127,7 +127,7 @@ Specific fields in the specification can be extracted in YAML format:
     end
 
     unless options[:all] then
-      specs = [specs.sort_by { |s| s.version }.last]
+      specs = [specs.max_by { |s| s.version }]
     end
 
     specs.each do |s|

lib/rubygems/commands/unpack_command.rb

@@ -134,7 +134,7 @@ command help for an example.
 
     specs = dependency.matching_specs
 
-    selected = specs.sort_by { |s| s.version }.last # HACK: hunt last down
+    selected = specs.max_by { |s| s.version }
 
     return Gem::RemoteFetcher.fetcher.download_to_cache(dependency) unless
       selected

lib/rubygems/commands/update_command.rb

@@ -134,7 +134,7 @@ command to remove old versions.
       g.name == spec.name and g.match_platform?
     end
 
-    highest_remote_gem = matching_gems.sort_by { |g,_| g.version }.last
+    highest_remote_gem = matching_gems.max_by { |g,_| g.version }
 
     highest_remote_gem ||= [Gem::NameTuple.null]
 

lib/rubygems/dependency_resolver.rb

@@ -92,6 +92,32 @@ class Gem::DependencyResolver
     res.to_a
   end
 
+  ##
+  # Finds the State in +states+ that matches the +conflict+ so that we can try
+  # other possible sets.
+
+  def find_conflict_state conflict, states # :nodoc:
+    until states.empty? do
+      if conflict.for_spec? states.last.spec
+        state = states.last
+        state.conflicts << [state.spec, conflict]
+        return state
+      else
+        states.pop
+      end
+    end
+
+    nil
+  end
+
+  ##
+  # Extracts the specifications that may be able to fulfill +dependency+
+
+  def find_possible dependency # :nodoc:
+    possible = @set.find_all dependency
+    select_local_platforms possible
+  end
+
   def handle_conflict(dep, existing)
     # There is a conflict! We return the conflict
     # object which will be seen by the caller and be
@@ -144,118 +170,121 @@ class Gem::DependencyResolver
 
       # If there is already a spec activated for the requested name...
       if specs && existing = specs.find { |s| dep.name == s.name }
-
+        # then we're done since this new dep matches the existing spec.
-        # then we're done since this new dep matches the
-        # existing spec.
         next if dep.matches_spec? existing
 
         conflict = handle_conflict dep, existing
 
-        # Look through the state array and pop State objects
+        state = find_conflict_state conflict, states
-        # until we get back to the State that matches the conflict
-        # so that we can try other possible sets.
 
-        i = nil
+        return conflict unless state
 
-        until states.empty?
+        needed, specs = resolve_for_conflict needed, specs, state
-          if conflict.for_spec? states.last.spec
-            i = states.last
-            i.conflicts << [i.spec, conflict]
-            break
-          else
-            states.pop
-          end
-        end
 
-        if i
+        next
-          # We exhausted the possibles so it's definitely not going to
-          # work out, bail out.
-
-          if i.possibles.empty?
-            raise Gem::ImpossibleDependenciesError.new(i.dep, i.conflicts)
-          end
-
-          spec = i.possibles.pop
-
-          # Recursively call #resolve_for with this spec
-          # and add it's dependencies into the picture...
-
-          act = Gem::DependencyResolver::ActivationRequest.new spec, i.dep
-
-          needed = requests(spec, act, i.needed)
-          specs = Gem::List.prepend(i.specs, act)
-
-          next
-        else
-          return conflict
-        end
       end
 
-      # Get a list of all specs that satisfy dep and platform
+      possible = find_possible dep
-      possible = @set.find_all dep
-      possible = select_local_platforms possible
 
       case possible.size
       when 0
-        @missing << dep
+        resolve_for_zero dep
-
-        unless @soft_missing
-          # If there are none, then our work here is done.
-          raise Gem::UnsatisfiableDependencyError, dep
-        end
       when 1
-        # If there is one, then we just add it to specs
+        needed, specs =
-        # and process the specs dependencies by adding
+          resolve_for_single needed, specs, dep, possible
-        # them to needed.
-
-        spec = possible.first
-        act = Gem::DependencyResolver::ActivationRequest.new spec, dep, false
-
-        specs = Gem::List.prepend specs, act
-
-        # Put the deps for at the beginning of needed
-        # rather than the end to match the depth first
-        # searching done by the multiple case code below.
-        #
-        # This keeps the error messages consistent.
-        needed = requests(spec, act, needed)
       else
-        # There are multiple specs for this dep. This is
+        needed, specs =
-        # the case that this class is built to handle.
+          resolve_for_multiple needed, specs, states, dep, possible
-
-        # Sort them so that we try the highest versions
-        # first.
-        possible = possible.sort_by do |s|
-          [s.source, s.version, s.platform == Gem::Platform::RUBY ? -1 : 1]
-        end
-
-        # To figure out which to pick, we keep resolving
-        # given each one being activated and if there isn't
-        # a conflict, we know we've found a full set.
-        #
-        # We use an until loop rather than #reverse_each
-        # to keep the stack short since we're using a recursive
-        # algorithm.
-        #
-        spec = possible.pop
-
-        # We're may need to try all of +possible+, so we setup
-        # state to unwind back to current +needed+ and +specs+
-        # so we can try another. This is code is what makes the above
-        # code in conflict resolution possible.
-
-        act = Gem::DependencyResolver::ActivationRequest.new spec, dep
-
-        states << State.new(needed, specs, dep, spec, possible, [])
-
-        needed = requests(spec, act, needed)
-        specs = Gem::List.prepend(specs, act)
       end
     end
 
     specs
   end
 
+  ##
+  # Rewinds +needed+ and +specs+ to a previous state in +state+ for a conflict
+  # between +dep+ and +existing+.
+
+  def resolve_for_conflict needed, specs, state # :nodoc:
+    # We exhausted the possibles so it's definitely not going to work out,
+    # bail out.
+    raise Gem::ImpossibleDependenciesError.new state.dep, state.conflicts if
+      state.possibles.empty?
+
+    spec = state.possibles.pop
+
+    # Retry resolution with this spec and add it's dependencies
+    act = Gem::DependencyResolver::ActivationRequest.new spec, state.dep
+
+    needed = requests spec, act, state.needed
+    specs = Gem::List.prepend state.specs, act
+
+    return needed, specs
+  end
+
+  ##
+  # There are multiple +possible+ specifications for this +dep+.  Updates
+  # +needed+, +specs+ and +states+ for further resolution of the +possible+
+  # choices.
+
+  def resolve_for_multiple needed, specs, states, dep, possible # :nodoc:
+    # Sort them so that we try the highest versions first.
+    possible = possible.sort_by do |s|
+      [s.source, s.version, s.platform == Gem::Platform::RUBY ? -1 : 1]
+    end
+
+    # To figure out which to pick, we keep resolving given each one being
+    # activated and if there isn't a conflict, we know we've found a full set.
+    #
+    # We use an until loop rather than reverse_each to keep the stack short
+    # since we're using a recursive algorithm.
+    spec = possible.pop
+
+    # We may need to try all of +possible+, so we setup state to unwind back
+    # to current +needed+ and +specs+ so we can try another. This is code is
+    # what makes conflict resolution possible.
+
+    act = Gem::DependencyResolver::ActivationRequest.new spec, dep
+
+    states << State.new(needed, specs, dep, spec, possible, [])
+
+    needed = requests spec, act, needed
+    specs = Gem::List.prepend specs, act
+
+    return needed, specs
+  end
+
+  ##
+  # Add the spec from the +possible+ list to +specs+ and process the spec's
+  # dependencies by adding them to +needed+.
+
+  def resolve_for_single needed, specs, dep, possible # :nodoc:
+    spec = possible.first
+    act = Gem::DependencyResolver::ActivationRequest.new spec, dep, false
+
+    specs = Gem::List.prepend specs, act
+
+    # Put the deps for at the beginning of needed
+    # rather than the end to match the depth first
+    # searching done by the multiple case code below.
+    #
+    # This keeps the error messages consistent.
+    needed = requests spec, act, needed
+
+    return needed, specs
+  end
+
+  ##
+  # When there are no possible specifications for +dep+ our work is done.
+
+  def resolve_for_zero dep # :nodoc:
+    @missing << dep
+
+    unless @soft_missing
+      raise Gem::UnsatisfiableDependencyError, dep
+    end
+  end
+
   ##
   # Returns the gems in +specs+ that match the local platform.
 

lib/rubygems/dependency_resolver/index_specification.rb

@@ -1,8 +1,7 @@
 ##
-# Represents a possible Specification object returned
+# Represents a possible Specification object returned from IndexSet.  Used to
-# from IndexSet. Used to delay needed to download full
+# delay needed to download full Specification objects when only the +name+
-# Specification objects when only the +name+ and +version+
+# and +version+ are needed.
-# are needed.
 
 class Gem::DependencyResolver::IndexSpecification
 

lib/rubygems/remote_fetcher.rb

@@ -107,7 +107,7 @@ class Gem::RemoteFetcher
 
     return if found.empty?
 
-    spec, source = found.sort_by { |(s,_)| s.version }.last
+    spec, source = found.max_by { |(s,_)| s.version }
 
     download spec, source.uri.to_s
   end

lib/rubygems/source/local.rb

@@ -88,7 +88,7 @@ class Gem::Source::Local < Gem::Source
       end
     end
 
-    found.sort_by { |s| s.version }.last
+    found.max_by { |s| s.version }
   end
 
   def fetch_spec(name)

lib/rubygems/specification.rb

@@ -1509,7 +1509,6 @@ class Gem::Specification < Gem::BasicSpecification
   #   [depending_gem, dependency, [list_of_gems_that_satisfy_dependency]]
 
   def dependent_gems
-    # REFACTOR: out = []; each; out; ? Really? No #collect love?
     out = []
     Gem::Specification.each do |spec|
       spec.dependencies.each do |dep|
@@ -1937,7 +1936,6 @@ class Gem::Specification < Gem::BasicSpecification
     q.group 2, 'Gem::Specification.new do |s|', 'end' do
       q.breakable
 
-      # REFACTOR: each_attr - use in to_yaml as well
       @@attributes.each do |attr_name|
         current_value = self.send attr_name
         if current_value != default_value(attr_name) or
@@ -2186,10 +2184,6 @@ class Gem::Specification < Gem::BasicSpecification
   # Returns a Ruby code representation of this specification, such that it can
   # be eval'ed and reconstruct the same specification later.  Attributes that
   # still have their default values are omitted.
-  #
-  # REFACTOR: This, plus stuff like #ruby_code and #pretty_print, should
-  # probably be extracted out into some sort of separate class. SRP, do you
-  # speak it!??!
 
   def to_ruby
     mark_version

lib/rubygems/version.rb

@@ -148,7 +148,7 @@ class Gem::Version
   # FIX: These are only used once, in .correct?. Do they deserve to be
   # constants?
   VERSION_PATTERN = '[0-9]+(?>\.[0-9a-zA-Z]+)*(-[0-9A-Za-z-]+(\.[0-9A-Za-z-]+)*)?' # :nodoc:
-  ANCHORED_VERSION_PATTERN = /\A\s*(#{VERSION_PATTERN})*\s*\z/ # :nodoc:
+  ANCHORED_VERSION_PATTERN = /\A\s*(#{VERSION_PATTERN})?\s*\z/ # :nodoc:
 
   ##
   # A string representation of this Version.

test/rubygems/test_gem_requirement.rb

@@ -47,18 +47,20 @@ class TestGemRequirement < Gem::TestCase
   end
 
   def test_parse_bad
-    e = assert_raises Gem::Requirement::BadRequirementError do
+    [
-      Gem::Requirement.parse nil
+      nil,
+      '',
+      '! 1',
+      '= junk',
+      '1..2',
+    ].each do |bad|
+      e = assert_raises Gem::Requirement::BadRequirementError do
+        Gem::Requirement.parse bad
+      end
+
+      assert_equal "Illformed requirement [#{bad.inspect}]", e.message
     end
 
-    assert_equal 'Illformed requirement [nil]', e.message
-
-    e = assert_raises Gem::Requirement::BadRequirementError do
-      Gem::Requirement.parse ""
-    end
-
-    assert_equal 'Illformed requirement [""]', e.message
-
     assert_equal Gem::Requirement::BadRequirementError.superclass, ArgumentError
   end
 

test/rubygems/test_gem_specification.rb

@@ -1112,6 +1112,20 @@ dependencies: []
     assert_equal [@bonobo, @monkey], @gem.dependencies
   end
 
+  def test_dependent_gems
+    util_setup_deps
+
+    assert_empty @gem.dependent_gems
+
+    bonobo = quick_spec 'bonobo'
+
+    expected = [
+      [@gem, @bonobo, [bonobo]],
+    ]
+
+    assert_equal expected, bonobo.dependent_gems
+  end
+
   def test_doc_dir
     assert_equal File.join(@gemhome, 'doc', 'a-1'), @a1.doc_dir
   end
@@ -1550,13 +1564,13 @@ dependencies: []
     @a2.add_runtime_dependency 'b', '1'
     @a2.dependencies.first.instance_variable_set :@type, nil
     @a2.required_rubygems_version = Gem::Requirement.new '> 0'
-    @a2.require_paths << "lib/a/ext"
+    @a2.require_paths << 'other'
 
     ruby_code = @a2.to_ruby
 
     expected = <<-SPEC
 # -*- encoding: utf-8 -*-
-# stub: a 2 ruby lib\0lib/a/ext
+# stub: a 2 ruby lib\0other
 
 Gem::Specification.new do |s|
   s.name = "a"
@@ -1569,7 +1583,7 @@ Gem::Specification.new do |s|
   s.email = "example@example.com"
   s.files = ["lib/code.rb"]
   s.homepage = "http://example.com"
-  s.require_paths = ["lib", "lib/a/ext"]
+  s.require_paths = ["lib", "other"]
   s.rubygems_version = "#{Gem::VERSION}"
   s.summary = "this is a summary"
 

test/rubygems/test_gem_version.rb

@@ -67,12 +67,17 @@ class TestGemVersion < Gem::TestCase
   end
 
   def test_initialize_bad
-    ["junk", "1.0\n2.0"].each do |bad|
+    %W[
-      e = assert_raises ArgumentError do
+      junk
+      1.0\n2.0
+      1..2
+      1.2\ 3.4
+    ].each do |bad|
+      e = assert_raises ArgumentError, bad do
         Gem::Version.new bad
       end
 
-      assert_equal "Malformed version number string #{bad}", e.message
+      assert_equal "Malformed version number string #{bad}", e.message, bad
     end
   end