[CHEF-3367] prevent accidental removal of chef-client daemon pidfile when child processes of the daemon exit

Children forked from the chef-client daemon (such as those triggered
during runs of only_if commands) inherit the at_exit handler of the
daemon, which removes the daemon's pid file.  remove_pid_file now
only removes the pid file if it's invoked by the same process which
created it in the first place.

2 files changed, 42 insertions, 6 deletions

diff --git a/chef/lib/chef/daemon.rb b/chef/lib/chef/daemon.rb
index bb5ccf753a..7989c5ac8a 100644
--- a/chef/lib/chef/daemon.rb
+++ b/chef/lib/chef/daemon.rb
@@ -46,7 +46,10 @@ class Chef
             $stdout.reopen("/dev/null", "a")
             $stderr.reopen($stdout)
             save_pid_file
-            at_exit { remove_pid_file }
+            at_exit {
+              Chef::Log.debug("daemon pid #{Process.pid} in at_exit handler")
+              remove_pid_file
+            }
           rescue NotImplementedError => e
             Chef::Application.fatal!("There is no fork: #{e.message}")
           end
@@ -107,7 +110,9 @@ class Chef
         end
       
         begin
-          File.open(file, "w") { |f| f.write(Process.pid.to_s) }
+          pid = Process.pid.to_s
+          File.open(file, "w") { |f| f.write(pid) }
+          Chef::Log.debug("Wrote #{pid} to #{file}")
         rescue Errno::EACCES => e
           Chef::Application.fatal!("Couldn't write to pidfile #{file}, permission denied: #{e.message}")
         end
@@ -115,7 +120,17 @@ class Chef
     
       # Delete the PID from the filesystem
       def remove_pid_file
-        FileUtils.rm(pid_file) if File.exists?(pid_file)
+        return unless File.exists?(pid_file)
+
+        daemon_pid = pid_from_file
+        my_pid = Process.pid
+        if daemon_pid != my_pid
+          Chef::Log.debug("My pid is #{my_pid}; not removing #{pid_file} which contains #{daemon_pid}")
+          return
+        end
+
+        FileUtils.rm(pid_file)
+        Chef::Log.debug("Removed #{pid_file}")
       end
            
       # Change process user/group to those specified in Chef::Config
diff --git a/chef/spec/unit/daemon_spec.rb b/chef/spec/unit/daemon_spec.rb
index 1efdf2a2ad..6c0e5ee5f9 100644
--- a/chef/spec/unit/daemon_spec.rb
+++ b/chef/spec/unit/daemon_spec.rb
@@ -150,11 +150,32 @@ describe Chef::Daemon do
         File.stub!(:exists?).with("/var/run/chef/chef-client.pid").and_return(true)
       end
 
-      it "should remove the file" do
-        FileUtils.should_receive(:rm).with("/var/run/chef/chef-client.pid")
-        Chef::Daemon.remove_pid_file
+      describe "and contains the current pid" do
+
+        before do
+          File.stub!(:read).with("/var/run/chef/chef-client.pid").and_return("#$$\n")
+        end
+
+        it "should remove the file" do
+          FileUtils.should_receive(:rm).with("/var/run/chef/chef-client.pid")
+          Chef::Daemon.remove_pid_file
+        end
+
       end
 
+      describe "and contains a different pid" do
+
+        before do
+          other_pid = Process.pid + 1
+          File.stub!(:read).with("/var/run/chef/chef-client.pid").and_return("#{other_pid}\n")
+        end
+
+        it "should not remove the file" do
+          FileUtils.should_not_receive(:rm)
+          Chef::Daemon.remove_pid_file
+        end
+
+      end
 
     end