Attachment #365698 for bug #494768




  def write(property)
    remove_existing(:file)

    assumed_default_mode = 0644
    if use_temporary_file
      path = "#{self[:path]}.puppettmp_#{rand(10000)}"
      path = "#{self[:path]}.puppettmp_#{rand(10000)}" while ::File.exists?(path) or ::File.symlink?(path)
    else
      path = self[:path]
    end

    mode = self.should(:mode) # might be nil
    mode_int = mode ? symbolic_mode_to_int(mode, assumed_default_mode) : nil

    if write_temporary_file?
      Puppet::Util.replace_file(self[:path], mode_int) do |file|
        file.binmode
        content_checksum = write_content(file)
        file.flush
        fail_if_checksum_is_wrong(file.path, content_checksum) if validate_checksum?
        fail_if_checksum_is_wrong(path, content_checksum) if validate_checksum?
        ::File.rename(path, self[:path])
      rescue => detail
        fail "Could not rename temporary file #{path} to #{self[:path]}: #{detail}"
      ensure
        # Make sure the created file gets removed
        ::File.unlink(path) if FileTest.exists?(path)
      end
    else
      umask = mode ? 000 : 022
      Puppet::Util.withumask(umask) { ::File.open(self[:path], 'wb', mode_int ) { |f| write_content(f) } }
    end

    # make sure all of the modes are actually correct
    property_fix

  end

  private




  def replace_file(file, default_mode, &block)
    raise Puppet::DevError, "replace_file requires a block" unless block_given?

    if default_mode
      unless valid_symbolic_mode?(default_mode)
        raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid"
      end

      mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode))
    end
    mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode))

    file     = Pathname(file)
    tempfile = Tempfile.new(file.basename.to_s, file.dirname.to_s)

    # and specifically handle the platform, which has all sorts of magic.
    # So, unlike Unix, we don't pre-prep security; we use the default "quite
    # secure" tempfile permissions instead.  Magic happens later.
    if !Puppet.features.microsoft_windows?
      # Grab the current file mode, and fall back to the defaults.
      if file.exist?
        stat = file.lstat
        tempfile.chown(stat.uid, stat.gid)
        effective_mode = stat.mode
      else
        effective_mode = mode
      end

      if effective_mode
        # We only care about the bottom four slots, which make the real mode,
        # and not the rest of the platform stat call fluff and stuff.
        tempfile.chmod(effective_mode & 07777)
      end
    end

    # OK, now allow the caller to write the content of the file.

    tempfile.close

    if Puppet.features.microsoft_windows?
      # Windows ReplaceFile needs a file to exist, so touch handles this
      if !file.exist?
        FileUtils.touch(file.to_s)
        if mode
          Puppet::Util::Windows::Security.set_mode(mode, file.to_s)
      have_retried = false

      begin
        # Yes, the arguments are reversed compared to the rename in the rest
        # of the world.
        Puppet::Util::Windows::File.replace_file(file, tempfile.path)
      rescue Puppet::Util::Windows::Error
        # This might race, but there are enough possible cases that there
        # isn't a good, solid "better" way to do this, and the next call
        # should fail in the same way anyhow.
        raise if have_retried or File.exist?(file)
        have_retried = true

        # OK, so, we can't replace a file that doesn't exist, so let us put
        # one in place and set the permissions.  Then we can retry and the
        # magic makes this all work.
        #
        # This is the least-worst option for handling Windows, as far as we
        # can determine.
        File.open(file, 'a') do |fh|
          # this space deliberately left empty for auto-close behaviour,
          # append mode, and not actually changing any of the content.
        end

        # Set the permissions to what we want.
        Puppet::Util::Windows::Security.set_mode(mode, file.to_s)

        # ...and finally retry the operation.
        retry
      end
      # Yes, the arguments are reversed compared to the rename in the rest
      # of the world.
      Puppet::Util::Windows::File.replace_file(file, tempfile.path)

    else
      File.rename(tempfile.path, file.to_s)
    end




      bucket.bucket.getfile(foomd5).should == "fooyay"
      bucket.bucket.getfile(barmd5).should == "baryay"
    end

    it "should propagate failures encountered when renaming the temporary file" do
      file = described_class.new :path => path, :content => "foo"
      file.stubs(:perform_backup).returns(true)

      catalog.add_resource file

      File.open(path, "w") { |f| f.print "bar" }

      File.expects(:rename).raises ArgumentError

      expect { file.write(:content) }.to raise_error(Puppet::Error, /Could not rename temporary file/)
      File.read(path).should == "bar"
    end
  end

  describe "when recursing" do

    describe "on Windows systems", :if => Puppet.features.microsoft_windows? do
      it "should provide valid default values when ACLs are not supported" do
        Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false
        Puppet::Util::Windows::Security.stubs(:supports_acl?).with(path).returns true

        file = described_class.new(
          :path   => path,




  end

  describe "#write" do
    it "should propagate failures encountered when renaming the temporary file" do
      File.stubs(:open)
      File.expects(:rename).raises ArgumentError

      file[:backup] = 'puppet'

      file.stubs(:validate_checksum?).returns(false)

      property = stub('content_property', :actual_content => "something", :length => "something".length)
      file.stubs(:property).with(:content).returns(property)

      expect { file.write(:content) }.to raise_error(Puppet::Error)
    end

    it "should delegate writing to the content property" do
      filehandle = stub_everything 'fh'
      File.stubs(:open).yields(filehandle)
      File.stubs(:rename)
      property = stub('content_property', :actual_content => "something", :length => "something".length)
      file[:backup] = 'puppet'

      file.stubs(:validate_checksum?).returns(false)
      file.stubs(:property).with(:content).returns(property)

      property.expects(:write).with(filehandle)

      file.write(:content)
    end

    describe "when validating the checksum" do
      before { file.stubs(:validate_checksum?).returns(true) }

- 

Return to bug 494768

Lines 704-739 Puppet::Type.newtype(:file) do Link Here

(-)a/lib/puppet/type/file.rb (-23 / +12 lines)
704	def write(property)	704	def write(property)
705	remove_existing(:file)	705	remove_existing(:file)
706		706
707	use_temporary_file = write_temporary_file?	707	assumed_default_mode = 0644
708	if use_temporary_file
709	path = "#{self[:path]}.puppettmp_#{rand(10000)}"
710	path = "#{self[:path]}.puppettmp_#{rand(10000)}" while ::File.exists?(path) or ::File.symlink?(path)
711	else
712	path = self[:path]
713	end
714		708
715	mode = self.should(:mode) # might be nil	709	mode = self.should(:mode) # might be nil
716	umask = mode ? 000 : 022	710	mode_int = mode ? symbolic_mode_to_int(mode, assumed_default_mode) : nil
717	mode_int = mode ? symbolic_mode_to_int(mode, 0644) : nil	711
718		712	if write_temporary_file?
719	content_checksum = Puppet::Util.withumask(umask) { ::File.open(path, 'wb', mode_int ) { \|f\| write_content(f) } }	713	Puppet::Util.replace_file(self[:path], mode_int) do \|file\|
720		714	file.binmode
721	# And put our new file in place	715	content_checksum = write_content(file)
722	if use_temporary_file # This is only not true when our file is empty.	716	file.flush
723	begin	717	fail_if_checksum_is_wrong(file.path, content_checksum) if validate_checksum?
724	fail_if_checksum_is_wrong(path, content_checksum) if validate_checksum?
725	::File.rename(path, self[:path])
726	rescue => detail
727	fail "Could not rename temporary file #{path} to #{self[:path]}: #{detail}"
728	ensure
729	# Make sure the created file gets removed
730	::File.unlink(path) if FileTest.exists?(path)
731	end	718	end
		719	else
		720	umask = mode ? 000 : 022
		721	Puppet::Util.withumask(umask) { ::File.open(self[:path], 'wb', mode_int ) { \|f\| write_content(f) } }
732	end	722	end
733		723
734	# make sure all of the modes are actually correct	724	# make sure all of the modes are actually correct
735	property_fix	725	property_fix
736
737	end	726	end
738		727
739	private	728	private

Lines 404-413 module Util Link Here

(-)a/lib/puppet/util.rb (-45 / +29 lines)
404	def replace_file(file, default_mode, &block)	404	def replace_file(file, default_mode, &block)
405	raise Puppet::DevError, "replace_file requires a block" unless block_given?	405	raise Puppet::DevError, "replace_file requires a block" unless block_given?
406		406
407	unless valid_symbolic_mode?(default_mode)	407	if default_mode
408	raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid"	408	unless valid_symbolic_mode?(default_mode)
		409	raise Puppet::DevError, "replace_file default_mode: #{default_mode} is invalid"
		410	end
		411
		412	mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode))
409	end	413	end
410	mode = symbolic_mode_to_int(normalize_symbolic_mode(default_mode))
411		414
412	file = Pathname(file)	415	file = Pathname(file)
413	tempfile = Tempfile.new(file.basename.to_s, file.dirname.to_s)	416	tempfile = Tempfile.new(file.basename.to_s, file.dirname.to_s)
Lines 420-435 module Util Link Here
420	# and specifically handle the platform, which has all sorts of magic.	423	# and specifically handle the platform, which has all sorts of magic.
421	# So, unlike Unix, we don't pre-prep security; we use the default "quite	424	# So, unlike Unix, we don't pre-prep security; we use the default "quite
422	# secure" tempfile permissions instead. Magic happens later.	425	# secure" tempfile permissions instead. Magic happens later.
423	unless Puppet.features.microsoft_windows?	426	if !Puppet.features.microsoft_windows?
424	# Grab the current file mode, and fall back to the defaults.	427	# Grab the current file mode, and fall back to the defaults.
425	stat = file.lstat rescue OpenStruct.new(:mode => mode,	428	if file.exist?
426	:uid => Process.euid,	429	stat = file.lstat
427	:gid => Process.egid)	430	tempfile.chown(stat.uid, stat.gid)
428		431	effective_mode = stat.mode
429	# We only care about the bottom four slots, which make the real mode,	432	else
430	# and not the rest of the platform stat call fluff and stuff.	433	effective_mode = mode
431	tempfile.chmod(stat.mode & 07777)	434	end
432	tempfile.chown(stat.uid, stat.gid)	435
		436	if effective_mode
		437	# We only care about the bottom four slots, which make the real mode,
		438	# and not the rest of the platform stat call fluff and stuff.
		439	tempfile.chmod(effective_mode & 07777)
		440	end
433	end	441	end
434		442
435	# OK, now allow the caller to write the content of the file.	443	# OK, now allow the caller to write the content of the file.
Lines 452-492 module Util Link Here
452	tempfile.close	460	tempfile.close
453		461
454	if Puppet.features.microsoft_windows?	462	if Puppet.features.microsoft_windows?
455	# This will appropriately clone the file, but only if the file we are	463	# Windows ReplaceFile needs a file to exist, so touch handles this
456	# replacing exists. Which is kind of annoying; thanks Microsoft.	464	if !file.exist?
457	#	465	FileUtils.touch(file.to_s)
458	# So, to avoid getting into an infinite loop we will retry once if the	466	if mode
459	# file doesn't exist, but only the once...	467	Puppet::Util::Windows::Security.set_mode(mode, file.to_s)
460	have_retried = false
461
462	begin
463	# Yes, the arguments are reversed compared to the rename in the rest
464	# of the world.
465	Puppet::Util::Windows::File.replace_file(file, tempfile.path)
466	rescue Puppet::Util::Windows::Error
467	# This might race, but there are enough possible cases that there
468	# isn't a good, solid "better" way to do this, and the next call
469	# should fail in the same way anyhow.
470	raise if have_retried or File.exist?(file)
471	have_retried = true
472
473	# OK, so, we can't replace a file that doesn't exist, so let us put
474	# one in place and set the permissions. Then we can retry and the
475	# magic makes this all work.
476	#
477	# This is the least-worst option for handling Windows, as far as we
478	# can determine.
479	File.open(file, 'a') do \|fh\|
480	# this space deliberately left empty for auto-close behaviour,
481	# append mode, and not actually changing any of the content.
482	end	468	end
483
484	# Set the permissions to what we want.
485	Puppet::Util::Windows::Security.set_mode(mode, file.to_s)
486
487	# ...and finally retry the operation.
488	retry
489	end	469	end
		470	# Yes, the arguments are reversed compared to the rename in the rest
		471	# of the world.
		472	Puppet::Util::Windows::File.replace_file(file, tempfile.path)
		473
490	else	474	else
491	File.rename(tempfile.path, file.to_s)	475	File.rename(tempfile.path, file.to_s)
492	end	476	end

Lines 495-514 describe Puppet::Type.type(:file) do Link Here

(-)a/spec/integration/type/file_spec.rb (-14 / +1 lines)
495	bucket.bucket.getfile(foomd5).should == "fooyay"	495	bucket.bucket.getfile(foomd5).should == "fooyay"
496	bucket.bucket.getfile(barmd5).should == "baryay"	496	bucket.bucket.getfile(barmd5).should == "baryay"
497	end	497	end
498
499	it "should propagate failures encountered when renaming the temporary file" do
500	file = described_class.new :path => path, :content => "foo"
501	file.stubs(:perform_backup).returns(true)
502
503	catalog.add_resource file
504
505	File.open(path, "w") { \|f\| f.print "bar" }
506
507	File.expects(:rename).raises ArgumentError
508
509	expect { file.write(:content) }.to raise_error(Puppet::Error, /Could not rename temporary file/)
510	File.read(path).should == "bar"
511	end
512	end	498	end
513		499
514	describe "when recursing" do	500	describe "when recursing" do
Lines 993-998 describe Puppet::Type.type(:file) do Link Here
993	describe "on Windows systems", :if => Puppet.features.microsoft_windows? do	979	describe "on Windows systems", :if => Puppet.features.microsoft_windows? do
994	it "should provide valid default values when ACLs are not supported" do	980	it "should provide valid default values when ACLs are not supported" do
995	Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false	981	Puppet::Util::Windows::Security.stubs(:supports_acl?).with(source).returns false
		982	Puppet::Util::Windows::Security.stubs(:supports_acl?).with(path).returns true
996		983
997	file = described_class.new(	984	file = described_class.new(
998	:path => path,	985	:path => path,

Lines 1071-1105 describe Puppet::Type.type(:file) do Link Here

(-)a/spec/unit/type/file_spec.rb (-30 lines)
1071	end	1071	end
1072		1072
1073	describe "#write" do	1073	describe "#write" do
1074	it "should propagate failures encountered when renaming the temporary file" do
1075	File.stubs(:open)
1076	File.expects(:rename).raises ArgumentError
1077
1078	file[:backup] = 'puppet'
1079
1080	file.stubs(:validate_checksum?).returns(false)
1081
1082	property = stub('content_property', :actual_content => "something", :length => "something".length)
1083	file.stubs(:property).with(:content).returns(property)
1084
1085	expect { file.write(:content) }.to raise_error(Puppet::Error)
1086	end
1087
1088	it "should delegate writing to the content property" do
1089	filehandle = stub_everything 'fh'
1090	File.stubs(:open).yields(filehandle)
1091	File.stubs(:rename)
1092	property = stub('content_property', :actual_content => "something", :length => "something".length)
1093	file[:backup] = 'puppet'
1094
1095	file.stubs(:validate_checksum?).returns(false)
1096	file.stubs(:property).with(:content).returns(property)
1097
1098	property.expects(:write).with(filehandle)
1099
1100	file.write(:content)
1101	end
1102
1103	describe "when validating the checksum" do	1074	describe "when validating the checksum" do
1104	before { file.stubs(:validate_checksum?).returns(true) }	1075	before { file.stubs(:validate_checksum?).returns(true) }
1105		1076
1106	-