From 9ba0c8a22c6f9ca856851ef6c2d38242754a7a00 Mon Sep 17 00:00:00 2001 From: Brice Figureau Date: Sat, 16 Oct 2010 15:10:28 +0200 Subject: Fix #4923 - close process race when truncating existing file Using File.open(file, "w") calls open(2) with O_CREAT|O_TRUNC which means when the file exists it is immediately truncated. But the file is not locked yet, so another process can either write or read to the file, leading to file corruption. The fix is to truncate only when the file is exclusively locked. This can be done on some operating system with O_EXLOCK open(2) flag. I chose the more portable option of: * open * flock * truncate * write * close It might also be good to flush and fsync the file after writing it, otherwise in case of crash an incomplete file can stay on disk. Signed-off-by: Brice Figureau --- lib/puppet/util/file_locking.rb | 5 +++- spec/integration/util/file_locking_spec.rb | 42 ++++++++++++++++++++++-------- spec/unit/util/file_locking_spec.rb | 26 +++++++++++++++--- 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/lib/puppet/util/file_locking.rb b/lib/puppet/util/file_locking.rb index 8b194ed83..bd0181733 100644 --- a/lib/puppet/util/file_locking.rb +++ b/lib/puppet/util/file_locking.rb @@ -34,8 +34,11 @@ module Puppet::Util::FileLocking end Puppet::Util.sync(file).synchronize(Sync::EX) do - File.open(file, "w", mode) do |rf| + 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 + lrf.seek(0, IO::SEEK_SET) + lrf.truncate(0) yield lrf end end diff --git a/spec/integration/util/file_locking_spec.rb b/spec/integration/util/file_locking_spec.rb index 20c61d3d5..50613448b 100755 --- a/spec/integration/util/file_locking_spec.rb +++ b/spec/integration/util/file_locking_spec.rb @@ -5,28 +5,30 @@ Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f require 'puppet/util/file_locking' describe Puppet::Util::FileLocking do - it "should be able to keep file corruption from happening when there are multiple writers" do - file = Tempfile.new("puppetspec") - filepath = file.path - file.close!() - file = filepath - data = {:a => :b, :c => "A string", :d => "another string", :e => %w{an array of strings}} - File.open(file, "w") { |f| f.puts YAML.dump(data) } + before :each do + @file = Tempfile.new("puppetspec") + filepath = @file.path + @file.close!() + @file = filepath + @data = {:a => :b, :c => "A string", :d => "another string", :e => %w{an array of strings}} + File.open(@file, "w") { |f| f.puts YAML.dump(@data) } + end + it "should be able to keep file corruption from happening when there are multiple writers threads" do threads = [] sync = Sync.new 9.times { |a| threads << Thread.new { 9.times { |b| sync.synchronize(Sync::SH) { - Puppet::Util::FileLocking.readlock(file) { |f| - YAML.load(f.read).should == data + Puppet::Util::FileLocking.readlock(@file) { |f| + YAML.load(f.read).should == @data } } sleep 0.01 sync.synchronize(Sync::EX) { - Puppet::Util::FileLocking.writelock(file) { |f| - f.puts YAML.dump(data) + Puppet::Util::FileLocking.writelock(@file) { |f| + f.puts YAML.dump(@data) } } } @@ -34,4 +36,22 @@ describe Puppet::Util::FileLocking do } threads.each { |th| th.join } end + + it "should be able to keep file corruption from happening when there are multiple writers processes" do + unless Process.fork + 50.times { |b| + Puppet::Util::FileLocking.writelock(@file) { |f| + f.puts YAML.dump(@data) + } + sleep 0.01 + } + Kernel.exit! + end + + 50.times { |c| + Puppet::Util::FileLocking.readlock(@file) { |f| + YAML.load(f.read).should == @data + } + } + end end diff --git a/spec/unit/util/file_locking_spec.rb b/spec/unit/util/file_locking_spec.rb index 10051060c..476aa2caa 100755 --- a/spec/unit/util/file_locking_spec.rb +++ b/spec/unit/util/file_locking_spec.rb @@ -96,21 +96,21 @@ describe Puppet::Util::FileLocking do end it "should use any specified mode when opening the file" do - File.expects(:open).with("/file", "w", :mymode) + File.expects(:open).with("/file", File::Constants::CREAT | File::Constants::WRONLY , :mymode) Puppet::Util::FileLocking.writelock('/file', :mymode) end it "should use the mode of the existing file if no mode is specified" do File.expects(:stat).with("/file").returns(mock("stat", :mode => 0755)) - File.expects(:open).with("/file", "w", 0755) + File.expects(:open).with("/file", File::Constants::CREAT | File::Constants::WRONLY, 0755) Puppet::Util::FileLocking.writelock('/file') end it "should use 0600 as the mode if no mode is specified and the file does not exist" do File.expects(:stat).raises(Errno::ENOENT) - File.expects(:open).with("/file", "w", 0600) + File.expects(:open).with("/file", File::Constants::CREAT | File::Constants::WRONLY, 0600) Puppet::Util::FileLocking.writelock('/file') end @@ -130,6 +130,8 @@ describe Puppet::Util::FileLocking do lfh = mock 'locked_filehandle' fh.expects(:lock_exclusive).yields(lfh) + lfh.stubs(:seek) + lfh.stubs(:truncate) lfh.expects(:print).with "foo" Puppet::Util::FileLocking.writelock('/file') do |f| @@ -137,6 +139,22 @@ describe Puppet::Util::FileLocking do end end + it "should truncate the file under an exclusive lock" do + fh = mock 'fh' + File.expects(:open).yields fh + + lfh = mock 'locked_filehandle' + fh.expects(:lock_exclusive).yields(lfh) + + lfh.expects(:seek).with(0, IO::SEEK_SET) + lfh.expects(:truncate).with(0) + lfh.stubs(:print) + + Puppet::Util::FileLocking.writelock('/file') do |f| + f.print "foo" + end + end + it "should only work on regular files" do File.expects(:file?).with('/file').returns false proc { Puppet::Util::FileLocking.writelock('/file') }.should raise_error(ArgumentError) @@ -148,7 +166,7 @@ describe Puppet::Util::FileLocking do Puppet::Util.expects(:sync).with('/file').returns @sync File.expects(:exists?).with('/file').returns false - File.expects(:open).with('/file', 'w', 0600).once + File.expects(:open).with('/file', File::Constants::CREAT | File::Constants::WRONLY, 0600).once Puppet::Util::FileLocking.writelock('/file') end -- cgit