From 392504a1eb49820711fe605e795fff897b5f5fcf Mon Sep 17 00:00:00 2001 From: Markus Roberts Date: Tue, 1 Feb 2011 16:48:09 -0800 Subject: Fix to fix for #5755 -- backref serialization issues in zaml This commit addresses the original issue that the change reverted in the previous commit for #5755 was intended to fix by removing the special case on labels in emit (lables, even if they are never generated, are not "new lines" and thus @recent_nl should always be set to false when one is emitted). It also partially addresses a related issue wherein temporary strings generated when field names are constructed recycle their object_id (they are temporary) and thus cause incorrect back references. This commit "fixes" the problem by never generating backrefs to strings (treating them as immutable). It does not address other suspected issues such as thread safety durring serialization due to the use of class variables to store the seen-object hash or the use of object ids as "permanently unique" identifiers. Paired with: Daniel Pittman Advice & Commiseration: Jesse Wolfe --- lib/puppet/util/zaml.rb | 52 ++++++++++++++++++++++----------------------- spec/unit/util/zaml_spec.rb | 26 ++++++++++++++++++++++- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/lib/puppet/util/zaml.rb b/lib/puppet/util/zaml.rb index 6ac956565..14aa90e5f 100644 --- a/lib/puppet/util/zaml.rb +++ b/lib/puppet/util/zaml.rb @@ -91,7 +91,7 @@ class ZAML end def emit(s) @result << s - @recent_nl = false unless s.kind_of?(Label) + @recent_nl = false end def nl(s='') emit(@indent || "\n") unless @recent_nl @@ -223,32 +223,30 @@ class String gsub( /([\x80-\xFF])/ ) { |x| "\\x#{x.unpack("C")[0].to_s(16)}" } end def to_zaml(z) - z.first_time_only(self) { - num = '[-+]?(0x)?\d+\.?\d*' - case - when self == '' - z.emit('""') - # when self =~ /[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\xFF]/ - # z.emit("!binary |\n") - # z.emit([self].pack("m*")) - when ( - (self =~ /\A(true|false|yes|no|on|null|off|#{num}(:#{num})*|!|=|~)$/i) or - (self =~ /\A\n* /) or - (self =~ /[\s:]$/) or - (self =~ /^[>|][-+\d]*\s/i) or - (self[-1..-1] =~ /\s/) or - (self =~ /[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\xFF]/) or - (self =~ /[,\[\]\{\}\r\t]|:\s|\s#/) or - (self =~ /\A([-:?!#&*'"]|<<|%.+:.)/) - ) - z.emit("\"#{escaped_for_zaml}\"") - when self =~ /\n/ - if self[-1..-1] == "\n" then z.emit('|+') else z.emit('|-') end - z.nested { split("\n",-1).each { |line| z.nl; z.emit(line.chomp("\n")) } } - else - z.emit(self) - end - } + num = '[-+]?(0x)?\d+\.?\d*' + case + when self == '' + z.emit('""') + # when self =~ /[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\xFF]/ + # z.emit("!binary |\n") + # z.emit([self].pack("m*")) + when ( + (self =~ /\A(true|false|yes|no|on|null|off|#{num}(:#{num})*|!|=|~)$/i) or + (self =~ /\A\n* /) or + (self =~ /[\s:]$/) or + (self =~ /^[>|][-+\d]*\s/i) or + (self[-1..-1] =~ /\s/) or + (self =~ /[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\xFF]/) or + (self =~ /[,\[\]\{\}\r\t]|:\s|\s#/) or + (self =~ /\A([-:?!#&*'"]|<<|%.+:.)/) + ) + z.emit("\"#{escaped_for_zaml}\"") + when self =~ /\n/ + if self[-1..-1] == "\n" then z.emit('|+') else z.emit('|-') end + z.nested { split("\n",-1).each { |line| z.nl; z.emit(line.chomp("\n")) } } + else + z.emit(self) + end end end diff --git a/spec/unit/util/zaml_spec.rb b/spec/unit/util/zaml_spec.rb index 59590c571..782ebdb0b 100755 --- a/spec/unit/util/zaml_spec.rb +++ b/spec/unit/util/zaml_spec.rb @@ -53,8 +53,32 @@ describe "Pure ruby yaml implementation" do it "should handle references to Scalar in Hash" do str = "hello" data = { "one" => str, "two" => str } - data.to_yaml.should == "--- \n two: &id001 hello\n one: *id001" expect { YAML.load(data.to_yaml).should == data }.should_not raise_error end + + class Zaml_test_class_A + attr_reader :false,:true + def initialize + @false = @true = 7 + end + end + it "should not blow up when magic strings are used as field names" do + data = Zaml_test_class_A.new + data.to_yaml.should == %Q{--- !ruby/object:Zaml_test_class_A\n \"false\": 7\n \"true\": 7} + expect { + r = YAML.load(data.to_yaml) + r.class.should == data.class + r.true.should == data.true + r.false.should == data.false + }.should_not raise_error + end + + it "should not blow up on back references inside arrays" do + s = [1,2] + data = [s,s] + data.to_yaml.should == %Q{--- \n - &id001 \n - 1\n - 2\n - *id001} + expect { YAML.load(data.to_yaml).should == data }.should_not raise_error + end + end -- cgit From f9e2e2b7d76ec88d7ad2ed0ce663b4219b72bc96 Mon Sep 17 00:00:00 2001 From: Markus Roberts Date: Tue, 1 Feb 2011 17:54:24 -0800 Subject: Augmentation of tests for prior commit There is no good answer to tests that depend on the order of itteration over hashes. --- spec/unit/util/zaml_spec.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/spec/unit/util/zaml_spec.rb b/spec/unit/util/zaml_spec.rb index 782ebdb0b..fd506ea93 100755 --- a/spec/unit/util/zaml_spec.rb +++ b/spec/unit/util/zaml_spec.rb @@ -36,6 +36,10 @@ describe "Pure ruby yaml implementation" do end } + def set_of_lines(l) + l.split("\n").sort + end + it "should handle references to Array in Hash values correctly" do list = [1] data = { "one" => list, "two" => list } @@ -46,13 +50,15 @@ describe "Pure ruby yaml implementation" do it "should handle references to Hash in Hash values correctly" do hash = { 1 => 1 } data = { "one" => hash, "two" => hash } - data.to_yaml.should == "--- \n two: &id001 \n 1: 1\n one: *id001" + # This could still someday fail because the order change would also change which one got the back ref + set_of_lines(data.to_yaml).should == set_of_lines("--- \n two: &id001 \n 1: 1\n one: *id001") expect { YAML.load(data.to_yaml).should == data }.should_not raise_error end it "should handle references to Scalar in Hash" do str = "hello" data = { "one" => str, "two" => str } + set_of_lines(data.to_yaml).should == set_of_lines("--- \n two: hello\n one: hello") expect { YAML.load(data.to_yaml).should == data }.should_not raise_error end -- cgit