From 790fa9f8e2ffe927820a3585095f4e9a47f57391 Mon Sep 17 00:00:00 2001 From: Sam Phippen Date: Tue, 8 Sep 2015 17:15:51 +0100 Subject: [PATCH 1/2] Add a failing test for #5605 --- test/unit/vagrant/config/loader_test.rb | 29 +++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/unit/vagrant/config/loader_test.rb b/test/unit/vagrant/config/loader_test.rb index cd7a9e975..f13a7f5a6 100644 --- a/test/unit/vagrant/config/loader_test.rb +++ b/test/unit/vagrant/config/loader_test.rb @@ -38,6 +38,35 @@ describe Vagrant::Config::Loader do let(:instance) { described_class.new(versions, version_order) } + describe "#set" do + context "with an object that cannot be inspected" do + + # This represents the euro symbol in UTF-16LE. pack("c*") returns an ASCII + # string and so we have to force the encoding + UTF_16LE_STRING_THAT_CANNOT_BE_DOWNCAST_TO_ASCII = [0x20, 0xAC].pack("c*").force_encoding("UTF-16LE") + + + let(:klass_with_bad_inspect_string) do + Class.new do + def inspect + UTF_16LE_STRING_THAT_CANNOT_BE_DOWNCAST_TO_ASCII + end + end + end + + let(:test_source) { + Class.new do + def initialize(collaborator) + @foo = collaborator.new + end + end.new(klass_with_bad_inspect_string) + } + it "does not raise the ascii encoding exception" do + instance.set(:arbitrary, test_source) + end + end + end + describe "basic loading" do it "should ignore non-existent load order keys" do instance.load([:foo]) From eeb750cd33dcbc605990a6f8e33b7267dda43125 Mon Sep 17 00:00:00 2001 From: Sam Phippen Date: Tue, 8 Sep 2015 17:30:50 +0100 Subject: [PATCH 2/2] Catch encoding problems with sources provided to Vagrant::Config::Loader#set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Here we implement a naive solution to #5605 which catches the case that a provided source contains an object which cannot be inspected, because an object contained within in has an #inspect string that returns a string that is incompatible with the encoding in `Encoding.default_external` or a string which cannot be downcast to 7-bit ascii. The Ruby VM implementation of "#inspect" implements this checking on these lines of code: http://git.io/vZYNS. A Ruby level override of this method does not cause this problem. For example: ```ruby class Foo def inspect "😍".encode("UTF-16LE") end ``` will not cause the problem, because that's a Ruby implementation and the VM's checks don't occur. However, if we have an Object which **does** use the VM implementation of inspect, that contains an object that has an inspect string which returns non-ascii, we encounter the bug. For example: ```ruby class Bar def inspect "😍".encode("UTF-16LE") end end class Foo def initialize @bar = Bar.new end end Foo.new.inspect ``` Will cause the issue. The solution this patch provides basically catches the encoding error and inserts a string which attempts to help the user work out which object was provided without blowing up. Most likely, this was caused by a user having a weird encoding coming out of one of the sources passed in, but without a full repro case, it's not clear whether a patch should be applied to a different object in the system. Closes #5605. --- lib/vagrant/config/loader.rb | 22 ++++++++++++++++------ test/unit/vagrant/config/loader_test.rb | 5 ++++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/vagrant/config/loader.rb b/lib/vagrant/config/loader.rb index e3f80a579..ef4abe4bd 100644 --- a/lib/vagrant/config/loader.rb +++ b/lib/vagrant/config/loader.rb @@ -41,11 +41,21 @@ module Vagrant # `set` multiple times with the same name will override any previously # set values. In this way, the last set data for a given name wins. def set(name, sources) - @logger.info("Set #{name.inspect} = #{sources.inspect}") - # Sources should be an array sources = [sources] if !sources.kind_of?(Array) + reliably_inspected_sources = sources.reduce({}) { |accum, source| + begin + accum[source] = source.inspect + rescue Encoding::CompatibilityError + accum[source] = "" + end + + accum + } + + @logger.info("Set #{name.inspect} = #{reliably_inspected_sources.values}") + # Gather the procs for every source, since that is what we care about. procs = [] sources.each do |source| @@ -53,8 +63,8 @@ module Vagrant # Load the procs for this source and cache them. This caching # avoids the issue where a file may have side effects when loading # and loading it multiple times causes unexpected behavior. - @logger.debug("Populating proc cache for #{source.inspect}") - @proc_cache[source] = procs_for_source(source) + @logger.debug("Populating proc cache for #{reliably_inspected_sources[source]}") + @proc_cache[source] = procs_for_source(source, reliably_inspected_sources) end # Add on to the array of procs we're going to use @@ -164,7 +174,7 @@ module Vagrant # The `Proc` objects returned will expect a single argument for # the configuration object and are expected to mutate this # configuration object. - def procs_for_source(source) + def procs_for_source(source, reliably_inspected_sources) # Convert all pathnames to strings so we just have their path source = source.to_s if source.is_a?(Pathname) @@ -180,7 +190,7 @@ module Vagrant # Strings are considered paths, so load them return procs_for_path(source) else - raise ArgumentError, "Unknown configuration source: #{source.inspect}" + raise ArgumentError, "Unknown configuration source: #{reliably_inspected_sources[source]}" end end diff --git a/test/unit/vagrant/config/loader_test.rb b/test/unit/vagrant/config/loader_test.rb index f13a7f5a6..4073b85db 100644 --- a/test/unit/vagrant/config/loader_test.rb +++ b/test/unit/vagrant/config/loader_test.rb @@ -61,8 +61,11 @@ describe Vagrant::Config::Loader do end end.new(klass_with_bad_inspect_string) } + it "does not raise the ascii encoding exception" do - instance.set(:arbitrary, test_source) + expect { + instance.set(:arbitrary, test_source) + }.to raise_error(ArgumentError, /Unknown configuration source/) end end end