(#5931) Prevent errors when calling insync? on audited properties

Created a method safe_insync? which first checks whether the property
has a "should" value of nil, and if so returns true.  Most insync?
methods were already doing this, but a few were not, leading to bugs
if a property was being audited but not set.

Types should continue to override the insync? method, but callers of
insync? should call safe_insync? instead.

Paired-with: Jesse Wolfe <jesse@puppetlabs.com>

18 files changed, 28 insertions, 34 deletions

diff --git a/lib/puppet/property.rb b/lib/puppet/property.rb
index 84e1a0360..12f496a6e 100644
--- a/lib/puppet/property.rb
+++ b/lib/puppet/property.rb
@@ -152,9 +152,24 @@ class Puppet::Property < Puppet::Parameter
   # since we cannot fix it.  Otherwise, we expect our should value
   # to be an array, and if @is matches any of those values, then
   # we consider it to be in-sync.
-  def insync?(is)
+  #
+  # Don't override this method.
+  def safe_insync?(is)
+    # If there is no @should value, consider the property to be in sync.
     return true unless @should
 
+    # Otherwise delegate to the (possibly derived) insync? method.
+    insync?(is)
+  end
+
+  def self.method_added(sym)
+    raise "Puppet::Property#safe_insync? shouldn't be overridden; please override insync? instead" if sym == :safe_insync?
+  end
+
+  # This method should be overridden by derived classes if necessary
+  # to provide extra logic to determine whether the property is in
+  # sync.
+  def insync?(is)
     self.devfail "#{self.class.name}'s should is not array" unless @should.is_a?(Array)
 
     # an empty array is analogous to no should values
diff --git a/lib/puppet/property/keyvalue.rb b/lib/puppet/property/keyvalue.rb
index 0181708f9..57d0ea2d9 100644
--- a/lib/puppet/property/keyvalue.rb
+++ b/lib/puppet/property/keyvalue.rb
@@ -77,8 +77,6 @@ module Puppet
       end
 
       def insync?(is)
-        return true unless @should
-
         return true unless is
 
         (is == self.should)
diff --git a/lib/puppet/property/list.rb b/lib/puppet/property/list.rb
index dcee85db7..b86dc87f2 100644
--- a/lib/puppet/property/list.rb
+++ b/lib/puppet/property/list.rb
@@ -66,8 +66,6 @@ module Puppet
       end
 
       def insync?(is)
-        return true unless @should
-
         return true unless is
 
         (prepare_is_for_comparison(is) == self.should)
diff --git a/lib/puppet/provider/file/posix.rb b/lib/puppet/provider/file/posix.rb
index 415a5af40..f7b8c9797 100644
--- a/lib/puppet/provider/file/posix.rb
+++ b/lib/puppet/provider/file/posix.rb
@@ -28,8 +28,6 @@ Puppet::Type.type(:file).provide :posix do
   end
 
   def is_owner_insync?(current, should)
-    return true unless should
-
     should.each do |value|
       if value =~ /^\d+$/
         uid = Integer(value)
diff --git a/lib/puppet/provider/file/win32.rb b/lib/puppet/provider/file/win32.rb
index 23aa491dc..21e7ca974 100644
--- a/lib/puppet/provider/file/win32.rb
+++ b/lib/puppet/provider/file/win32.rb
@@ -15,8 +15,6 @@ Puppet::Type.type(:file).provide :microsoft_windows do
   end
 
   def is_owner_insync?(current, should)
-    return true unless should
-
     should.each do |value|
       if value =~ /^\d+$/
         uid = Integer(value)
diff --git a/lib/puppet/provider/zone/solaris.rb b/lib/puppet/provider/zone/solaris.rb
index c11444993..f46337b14 100644
--- a/lib/puppet/provider/zone/solaris.rb
+++ b/lib/puppet/provider/zone/solaris.rb
@@ -40,7 +40,7 @@ Puppet::Type.type(:zone).provide(:solaris) do
     # Then perform all of our configuration steps.  It's annoying
     # that we need this much internal info on the resource.
     @resource.send(:properties).each do |property|
-      str += property.configtext + "\n" if property.is_a? ZoneConfigProperty and ! property.insync?(properties[property.name])
+      str += property.configtext + "\n" if property.is_a? ZoneConfigProperty and ! property.safe_insync?(properties[property.name])
     end
 
     str += "commit\n"
diff --git a/lib/puppet/transaction/resource_harness.rb b/lib/puppet/transaction/resource_harness.rb
index 0abab10a5..4a3d35e0d 100644
--- a/lib/puppet/transaction/resource_harness.rb
+++ b/lib/puppet/transaction/resource_harness.rb
@@ -52,13 +52,13 @@ class Puppet::Transaction::ResourceHarness
     # Update the machine state & create logs/events
     events = []
     ensure_param = resource.parameter(:ensure)
-    if desired_values[:ensure] && !ensure_param.insync?(current_values[:ensure])
+    if desired_values[:ensure] && !ensure_param.safe_insync?(current_values[:ensure])
       events << apply_parameter(ensure_param, current_values[:ensure], audited_params.include?(:ensure), historical_values[:ensure])
       synced_params << :ensure
     elsif current_values[:ensure] != :absent
       work_order = resource.properties # Note: only the resource knows what order to apply changes in
       work_order.each do |param|
-        if desired_values[param.name] && !param.insync?(current_values[param.name])
+        if desired_values[param.name] && !param.safe_insync?(current_values[param.name])
           events << apply_parameter(param, current_values[param.name], audited_params.include?(param.name), historical_values[param.name])
           synced_params << param.name
         end
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index ea3944b4e..e03650b54 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -648,7 +648,7 @@ class Type
           "The is value is not in the is array for '#{property.name}'"
       end
       ensureis = is[property]
-      if property.insync?(ensureis) and property.should == :absent
+      if property.safe_insync?(ensureis) and property.should == :absent
         return true
       end
     end
@@ -660,7 +660,7 @@ class Type
       end
 
       propis = is[property]
-      unless property.insync?(propis)
+      unless property.safe_insync?(propis)
         property.debug("Not in sync: #{propis.inspect} vs #{property.should.inspect}")
         insync = false
       #else
diff --git a/lib/puppet/type/cron.rb b/lib/puppet/type/cron.rb
index 76399d693..4b18e71f9 100755
--- a/lib/puppet/type/cron.rb
+++ b/lib/puppet/type/cron.rb
@@ -54,11 +54,7 @@ Puppet::Type.newtype(:cron) do
     # We have to override the parent method, because we consume the entire
     # "should" array
     def insync?(is)
-      if @should
-        self.is_to_s(is) == self.should_to_s
-      else
-        true
-      end
+      self.is_to_s(is) == self.should_to_s
     end
 
     # A method used to do parameter input handling.  Converts integers
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index eee948cd5..0d69446b4 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -779,7 +779,7 @@ Puppet::Type.newtype(:file) do
       # Make sure we get a new stat objct
       expire
       currentvalue = thing.retrieve
-      thing.sync unless thing.insync?(currentvalue)
+      thing.sync unless thing.safe_insync?(currentvalue)
     end
   end
 end
diff --git a/lib/puppet/type/file/content.rb b/lib/puppet/type/file/content.rb
index b8f30a9c7..04b917a7e 100755
--- a/lib/puppet/type/file/content.rb
+++ b/lib/puppet/type/file/content.rb
@@ -96,7 +96,6 @@ module Puppet
       end
 
       return true if ! @resource.replace?
-      return true unless self.should
 
       result = super
 
diff --git a/lib/puppet/type/file/target.rb b/lib/puppet/type/file/target.rb
index 9e7229dda..b9fe9213b 100644
--- a/lib/puppet/type/file/target.rb
+++ b/lib/puppet/type/file/target.rb
@@ -14,7 +14,7 @@ module Puppet
 
       # Only call mklink if ensure didn't call us in the first place.
       currentensure  = @resource.property(:ensure).retrieve
-      mklink if @resource.property(:ensure).insync?(currentensure)
+      mklink if @resource.property(:ensure).safe_insync?(currentensure)
     end
 
     # Create our link.
diff --git a/lib/puppet/type/mount.rb b/lib/puppet/type/mount.rb
index d048c90f1..36fb553f5 100755
--- a/lib/puppet/type/mount.rb
+++ b/lib/puppet/type/mount.rb
@@ -89,7 +89,7 @@ module Puppet
           if prop.name == :ensure
             false
           else
-            ! prop.insync?(currentvalues[prop])
+            ! prop.safe_insync?(currentvalues[prop])
           end
         end.each { |prop| prop.sync }.length
         @resource.flush if oos > 0
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index 51a866332..d73d90dff 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -109,8 +109,6 @@ module Puppet
       # Override the parent method, because we've got all kinds of
       # funky definitions of 'in sync'.
       def insync?(is)
-        @should ||= []
-
         @latest ||= nil
         @lateststamp ||= (Time.now.to_i - 1000)
         # Iterate across all of the should values, and see how they
diff --git a/lib/puppet/type/service.rb b/lib/puppet/type/service.rb
index c00f02789..973a76c2d 100644
--- a/lib/puppet/type/service.rb
+++ b/lib/puppet/type/service.rb
@@ -73,7 +73,7 @@ module Puppet
 
         if property = @resource.property(:enable)
           val = property.retrieve
-          property.sync unless property.insync?(val)
+          property.sync unless property.safe_insync?(val)
         end
 
         event
diff --git a/lib/puppet/type/user.rb b/lib/puppet/type/user.rb
index 761d5d71b..5de73e3dd 100755
--- a/lib/puppet/type/user.rb
+++ b/lib/puppet/type/user.rb
@@ -112,8 +112,6 @@ module Puppet
       end
 
       def insync?(is)
-        return true unless self.should
-
         # We know the 'is' is a number, so we need to convert the 'should' to a number,
         # too.
         @should.each do |value|
diff --git a/lib/puppet/type/yumrepo.rb b/lib/puppet/type/yumrepo.rb
index 160b2164d..9b4c79428 100644
--- a/lib/puppet/type/yumrepo.rb
+++ b/lib/puppet/type/yumrepo.rb
@@ -7,14 +7,14 @@ module Puppet
   class IniProperty < Puppet::Property
     def insync?(is)
       # A should property of :absent is the same as nil
-      if is.nil? && (should.nil? || should == :absent)
+      if is.nil? && should == :absent
         return true
       end
       super(is)
     end
 
     def sync
-      if insync?(retrieve)
+      if safe_insync?(retrieve)
         result = nil
       else
         result = set(self.should)
diff --git a/lib/puppet/type/zpool.rb b/lib/puppet/type/zpool.rb
index 49cce552a..df06522e8 100755
--- a/lib/puppet/type/zpool.rb
+++ b/lib/puppet/type/zpool.rb
@@ -8,8 +8,6 @@ module Puppet
       end
 
       def insync?(is)
-        return true unless self.should
-
         return @should == [:absent] if is == :absent
 
         flatten_and_sort(is) == flatten_and_sort(@should)
@@ -18,8 +16,6 @@ module Puppet
 
     class MultiVDev < VDev
       def insync?(is)
-        return true unless self.should
-
         return @should == [:absent] if is == :absent
 
         return false unless is.length == @should.length