From 8eb39185810a59ad8d3aa874ba8f6c9a7b0949ac Mon Sep 17 00:00:00 2001 From: drbrain Date: Wed, 25 Sep 2013 00:53:19 +0000 Subject: [PATCH] * 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 | 6 + .../commands/specification_command.rb | 2 +- lib/rubygems/commands/unpack_command.rb | 2 +- lib/rubygems/commands/update_command.rb | 2 +- lib/rubygems/dependency_resolver.rb | 213 ++++++++++-------- .../index_specification.rb | 7 +- lib/rubygems/remote_fetcher.rb | 2 +- lib/rubygems/source/local.rb | 2 +- lib/rubygems/specification.rb | 6 - lib/rubygems/version.rb | 2 +- test/rubygems/test_gem_requirement.rb | 22 +- test/rubygems/test_gem_specification.rb | 20 +- test/rubygems/test_gem_version.rb | 11 +- 13 files changed, 173 insertions(+), 124 deletions(-) diff --git a/ChangeLog b/ChangeLog index 48d9b85da0..50fa3bae0d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +Wed Sep 25 09:53:11 2013 Eric Hodel + + * lib/rubygems: Fix CVE-2013-4363. Miscellaneous minor improvements. + + * test/rubygems: Tests for the above. + Tue Sep 24 17:38:56 2013 Nobuyoshi Nakada * string.c (rb_str_inspect): get rid of out-of-bound access. diff --git a/lib/rubygems/commands/specification_command.rb b/lib/rubygems/commands/specification_command.rb index d96c8b8627..3bc02a9c14 100644 --- a/lib/rubygems/commands/specification_command.rb +++ b/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| diff --git a/lib/rubygems/commands/unpack_command.rb b/lib/rubygems/commands/unpack_command.rb index e60e7d90fd..5a05ad0a81 100644 --- a/lib/rubygems/commands/unpack_command.rb +++ b/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 diff --git a/lib/rubygems/commands/update_command.rb b/lib/rubygems/commands/update_command.rb index e53798db86..401698196d 100644 --- a/lib/rubygems/commands/update_command.rb +++ b/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] diff --git a/lib/rubygems/dependency_resolver.rb b/lib/rubygems/dependency_resolver.rb index 9f3af38ae5..bbb5408a55 100644 --- a/lib/rubygems/dependency_resolver.rb +++ b/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 - # until we get back to the State that matches the conflict - # so that we can try other possible sets. + state = find_conflict_state conflict, states - i = nil + return conflict unless state - until states.empty? - if conflict.for_spec? states.last.spec - i = states.last - i.conflicts << [i.spec, conflict] - break - else - states.pop - end - end + needed, specs = resolve_for_conflict needed, specs, state - if i - # 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 + next end - # Get a list of all specs that satisfy dep and platform - possible = @set.find_all dep - possible = select_local_platforms possible + possible = find_possible dep case possible.size when 0 - @missing << dep - - unless @soft_missing - # If there are none, then our work here is done. - raise Gem::UnsatisfiableDependencyError, dep - end + resolve_for_zero dep when 1 - # If there is one, then we just add it to specs - # and process the specs dependencies by adding - # 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) + needed, specs = + resolve_for_single needed, specs, dep, possible else - # There are multiple specs for this dep. This is - # the case that this class is built to handle. - - # 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) + needed, specs = + resolve_for_multiple needed, specs, states, dep, possible 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. diff --git a/lib/rubygems/dependency_resolver/index_specification.rb b/lib/rubygems/dependency_resolver/index_specification.rb index fc409334e8..44a8438024 100644 --- a/lib/rubygems/dependency_resolver/index_specification.rb +++ b/lib/rubygems/dependency_resolver/index_specification.rb @@ -1,8 +1,7 @@ ## -# Represents a possible Specification object returned -# from IndexSet. Used to delay needed to download full -# Specification objects when only the +name+ and +version+ -# are needed. +# Represents a possible Specification object returned from IndexSet. Used to +# delay needed to download full Specification objects when only the +name+ +# and +version+ are needed. class Gem::DependencyResolver::IndexSpecification diff --git a/lib/rubygems/remote_fetcher.rb b/lib/rubygems/remote_fetcher.rb index 6abd6bd9db..bab96cba1a 100644 --- a/lib/rubygems/remote_fetcher.rb +++ b/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 diff --git a/lib/rubygems/source/local.rb b/lib/rubygems/source/local.rb index 642dda1e44..f7c70de6f0 100644 --- a/lib/rubygems/source/local.rb +++ b/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) diff --git a/lib/rubygems/specification.rb b/lib/rubygems/specification.rb index 6de8c3bc77..0952cb25e1 100644 --- a/lib/rubygems/specification.rb +++ b/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 diff --git a/lib/rubygems/version.rb b/lib/rubygems/version.rb index 2e546462d4..2ee887e213 100644 --- a/lib/rubygems/version.rb +++ b/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. diff --git a/test/rubygems/test_gem_requirement.rb b/test/rubygems/test_gem_requirement.rb index 1de0f41f20..01db08e84f 100644 --- a/test/rubygems/test_gem_requirement.rb +++ b/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 diff --git a/test/rubygems/test_gem_specification.rb b/test/rubygems/test_gem_specification.rb index 51c7be813f..2e2074fb29 100644 --- a/test/rubygems/test_gem_specification.rb +++ b/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" diff --git a/test/rubygems/test_gem_version.rb b/test/rubygems/test_gem_version.rb index 2ba196e48d..2136034d1f 100644 --- a/test/rubygems/test_gem_version.rb +++ b/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| - e = assert_raises ArgumentError do + %W[ + 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