Fix for #4955 -- Race condition & memory leak in Puppet::Util

The Puppet::Util.sync method was not thread safe and also leaked memory. I'm
not certain, but I believe the first is ironic and the second is merely a bug.

This patch addresses the problem by 1) refactoring so the sync objects
are never returned (and thus no one can cache a reference to one) 2) adding
reference counting 3) deleting them when they are no longer needed 4) doing
the thread safty dance.

It wasn't the first (or even second) solution considered, but it's the one
that I was able to make work in a way that I'm convinced is correct.  Its
main advantage is that it puts all the tricky bits in one place.

7 files changed, 34 insertions, 54 deletions

diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index ad0edd3b9..c76d63a54 100755
--- a/lib/puppet/daemon.rb
+++ b/lib/puppet/daemon.rb
@@ -43,7 +43,7 @@ class Puppet::Daemon
   # Create a pidfile for our daemon, so we can be stopped and others
   # don't try to start.
   def create_pidfile
-    Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do
+    Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do
       raise "Could not create PID file: #{pidfile}" unless Puppet::Util::Pidlock.new(pidfile).lock
     end
   end
@@ -73,7 +73,7 @@ class Puppet::Daemon
 
   # Remove the pid file for our daemon.
   def remove_pidfile
-    Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do
+    Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do
       locker = Puppet::Util::Pidlock.new(pidfile)
       locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if locker.locked?
     end
diff --git a/lib/puppet/network/server.rb b/lib/puppet/network/server.rb
index d424b9c4a..e4de07dea 100644
--- a/lib/puppet/network/server.rb
+++ b/lib/puppet/network/server.rb
@@ -32,14 +32,14 @@ class Puppet::Network::Server
   # Create a pidfile for our daemon, so we can be stopped and others
   # don't try to start.
   def create_pidfile
-    Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do
+    Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do
       raise "Could not create PID file: #{pidfile}" unless Puppet::Util::Pidlock.new(pidfile).lock
     end
   end
 
   # Remove the pid file for our daemon.
   def remove_pidfile
-    Puppet::Util.sync(Puppet[:name]).synchronize(Sync::EX) do
+    Puppet::Util.synchronize_on(Puppet[:name],Sync::EX) do
       locker = Puppet::Util::Pidlock.new(pidfile)
       locker.unlock or Puppet.err "Could not remove PID file #{pidfile}" if locker.locked?
     end
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index f2eaf0d06..1a5acaf22 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -3,6 +3,7 @@
 require 'puppet/util/monkey_patches'
 require 'sync'
 require 'puppet/external/lock'
+require 'monitor'
 
 module Puppet
   # A command failed to execute.
@@ -17,8 +18,7 @@ module Util
   require 'puppet/util/posix'
   extend Puppet::Util::POSIX
 
-  # Create a hash to store the different sync objects.
-  @@syncresources = {}
+  @@sync_objects = {}.extend MonitorMixin
 
   def self.activerecord_version
     if (defined?(::ActiveRecord) and defined?(::ActiveRecord::VERSION) and defined?(::ActiveRecord::VERSION::MAJOR) and defined?(::ActiveRecord::VERSION::MINOR))
@@ -28,10 +28,19 @@ module Util
     end
   end
 
-  # Return the sync object associated with a given resource.
-  def self.sync(resource)
-    @@syncresources[resource] ||= Sync.new
-    @@syncresources[resource]
+  
+  def self.synchronize_on(x,type)
+    sync_object,users = 0,1
+    begin
+      @@sync_objects.synchronize { 
+        (@@sync_objects[x] ||= [Sync.new,0])[users] += 1
+      }
+      @@sync_objects[x][sync_object].synchronize(type) { yield }
+    ensure
+      @@sync_objects.synchronize { 
+        @@sync_objects.delete(x) unless (@@sync_objects[x][users] -= 1) > 0
+      }
+    end
   end
 
   # Change the process to a different user
@@ -359,9 +368,7 @@ module Util
 
   # Create an exclusive lock.
   def threadlock(resource, type = Sync::EX)
-    Puppet::Util.sync(resource).synchronize(type) do
-      yield
-    end
+    Puppet::Util.synchronize_on(resource,type) { yield }
   end
 
   # Because some modules provide their own version of this method.
diff --git a/lib/puppet/util/file_locking.rb b/lib/puppet/util/file_locking.rb
index bd0181733..18744cab7 100644
--- a/lib/puppet/util/file_locking.rb
+++ b/lib/puppet/util/file_locking.rb
@@ -6,7 +6,7 @@ module Puppet::Util::FileLocking
   # Create a shared lock for reading
   def readlock(file)
     raise ArgumentError, "#{file} is not a file" unless !File.exists?(file) or File.file?(file)
-    Puppet::Util.sync(file).synchronize(Sync::SH) do
+    Puppet::Util.synchronize_on(file,Sync::SH) do
       File.open(file) { |f|
         f.lock_shared { |lf| yield lf }
       }
@@ -33,7 +33,7 @@ module Puppet::Util::FileLocking
       end
     end
 
-    Puppet::Util.sync(file).synchronize(Sync::EX) do
+    Puppet::Util.synchronize_on(file,Sync::EX) do
       File.open(file, File::Constants::CREAT | File::Constants::WRONLY, mode) do |rf|
         rf.lock_exclusive do |lrf|
           # poor's man open(2) O_EXLOCK|O_TRUNC
diff --git a/spec/unit/daemon_spec.rb b/spec/unit/daemon_spec.rb
index 15320736c..e24db7881 100755
--- a/spec/unit/daemon_spec.rb
+++ b/spec/unit/daemon_spec.rb
@@ -1,4 +1,4 @@
-#!/usr/bin/env ruby"
+#!/usr/bin/env ruby
 
 require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/daemon'
@@ -142,11 +142,7 @@ describe Puppet::Daemon do
   describe "when creating its pidfile" do
     it "should use an exclusive mutex" do
       Puppet.settings.expects(:value).with(:name).returns "me"
-
-      sync = mock 'sync'
-      Puppet::Util.expects(:sync).with("me").returns sync
-
-      sync.expects(:synchronize).with(Sync::EX)
+      Puppet::Util.expects(:synchronize_on).with("me",Sync::EX)
       @daemon.create_pidfile
     end
 
@@ -180,10 +176,8 @@ describe Puppet::Daemon do
     it "should use an exclusive mutex" do
       Puppet.settings.expects(:value).with(:name).returns "me"
 
-      sync = mock 'sync'
-      Puppet::Util.expects(:sync).with("me").returns sync
+      Puppet::Util.expects(:synchronize_on).with("me",Sync::EX)
 
-      sync.expects(:synchronize).with(Sync::EX)
       @daemon.remove_pidfile
     end
 
diff --git a/spec/unit/network/server_spec.rb b/spec/unit/network/server_spec.rb
index ccd9c082e..c2496dcca 100755
--- a/spec/unit/network/server_spec.rb
+++ b/spec/unit/network/server_spec.rb
@@ -5,6 +5,7 @@
 
 require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/network/server'
+require 'puppet/network/handler'
 
 describe Puppet::Network::Server do
   before do
@@ -161,11 +162,7 @@ describe Puppet::Network::Server do
   describe "when creating its pidfile" do
     it "should use an exclusive mutex" do
       Puppet.settings.expects(:value).with(:name).returns "me"
-
-      sync = mock 'sync'
-      Puppet::Util.expects(:sync).with("me").returns sync
-
-      sync.expects(:synchronize).with(Sync::EX)
+      Puppet::Util.expects(:synchronize_on).with("me",Sync::EX)
       @server.create_pidfile
     end
 
@@ -198,11 +195,7 @@ describe Puppet::Network::Server do
   describe "when removing its pidfile" do
     it "should use an exclusive mutex" do
       Puppet.settings.expects(:value).with(:name).returns "me"
-
-      sync = mock 'sync'
-      Puppet::Util.expects(:sync).with("me").returns sync
-
-      sync.expects(:synchronize).with(Sync::EX)
+      Puppet::Util.expects(:synchronize_on).with("me",Sync::EX)
       @server.remove_pidfile
     end
 
diff --git a/spec/unit/util/file_locking_spec.rb b/spec/unit/util/file_locking_spec.rb
index 476aa2caa..8fafb1d52 100755
--- a/spec/unit/util/file_locking_spec.rb
+++ b/spec/unit/util/file_locking_spec.rb
@@ -32,17 +32,12 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should use a global shared mutex" do
-      @sync = mock 'sync'
-      @sync.expects(:synchronize).with(Sync::SH).once
-      Puppet::Util.expects(:sync).with('/file').returns @sync
-
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).once
       Puppet::Util::FileLocking.readlock '/file'
     end
 
     it "should use a shared lock on the file" do
-      @sync = mock 'sync'
-      @sync.stubs(:synchronize).yields
-      Puppet::Util.expects(:sync).with('/file').returns @sync
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).yields
 
       fh = mock 'filehandle'
       File.expects(:open).with("/file").yields fh
@@ -59,9 +54,7 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should create missing files" do
-      @sync = mock 'sync'
-      @sync.stubs(:synchronize).yields
-      Puppet::Util.expects(:sync).with('/file').returns @sync
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).yields
 
       File.expects(:exists?).with('/file').returns false
       File.expects(:open).with('/file').once
@@ -72,9 +65,7 @@ describe Puppet::Util::FileLocking do
 
   describe "when acquiring a write lock" do
     before do
-      @sync = mock 'sync'
-      Puppet::Util.stubs(:sync).returns @sync
-      @sync.stubs(:synchronize).yields
+      Puppet::Util.stubs(:synchronize_on).yields
       File.stubs(:file?).with('/file').returns true
       File.stubs(:exists?).with('/file').returns true
     end
@@ -88,10 +79,7 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should use a global exclusive mutex" do
-      sync = mock 'sync'
-      sync.expects(:synchronize).with(Sync::EX)
-      Puppet::Util.expects(:sync).with("/file").returns sync
-
+      Puppet::Util.expects(:synchronize_on).with("/file",Sync::EX)
       Puppet::Util::FileLocking.writelock '/file'
     end
 
@@ -161,9 +149,7 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should create missing files" do
-      @sync = mock 'sync'
-      @sync.stubs(:synchronize).yields
-      Puppet::Util.expects(:sync).with('/file').returns @sync
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::EX).yields
 
       File.expects(:exists?).with('/file').returns false
       File.expects(:open).with('/file', File::Constants::CREAT | File::Constants::WRONLY, 0600).once