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 <brice-puppet@daysofwonder.com>

1 files changed, 31 insertions, 11 deletions

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