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. --- spec/unit/daemon_spec.rb | 12 +++--------- spec/unit/network/server_spec.rb | 13 +++---------- spec/unit/util/file_locking_spec.rb | 26 ++++++-------------------- 3 files changed, 12 insertions(+), 39 deletions(-) (limited to 'spec') 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 -- cgit