From ee61b4ecec7d5a993eac6a356b4bc0dcc6ceaf94 Mon Sep 17 00:00:00 2001 From: Markus Roberts Date: Mon, 18 Oct 2010 15:55:41 -0700 Subject: 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. --- lib/puppet/util/file_locking.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'lib/puppet/util') 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 -- cgit