Roll back commit a34c4febbbdfba6c045598101ca9a491cfde7dd9 in favor of not verifying the identity of the process killed on interrupt.

When the server dies, the client follows soon after, so this is kind of OK.

--
MOS_MIGRATED_REVID=133110872

8 files changed, 83 insertions, 102 deletions

diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index 018e64a7af..47a87910a0 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -1021,8 +1021,9 @@ static void StartServerAndConnect(BlazeServer *server) {
   // disaster.
   int server_pid = GetServerPid(server_dir);
   if (server_pid > 0) {
-    if (KillServerProcess(server_pid, globals->options.output_base,
-                          globals->options.install_base)) {
+    if (VerifyServerProcess(server_pid, globals->options.output_base,
+                            globals->options.install_base) &&
+        KillServerProcess(server_pid)) {
       fprintf(stderr, "Killed non-responsive server process (pid=%d)\n",
               server_pid);
     }
@@ -1438,16 +1439,38 @@ static void EnsureCorrectRunningVersion(BlazeServer* server) {
   }
 }
 
+// A signal-safe version of fprintf(stderr, ...).
+//
+// WARNING: any output from the blaze client may be interleaved
+// with output from the blaze server.  In --curses mode,
+// the Blaze server often erases the previous line of output.
+// So, be sure to end each such message with TWO newlines,
+// otherwise it may be erased by the next message from the
+// Blaze server.
+// Also, it's a good idea to start each message with a newline,
+// in case the Blaze server has written a partial line.
+static void sigprintf(const char *format, ...) {
+  char buf[1024];
+  va_list ap;
+  va_start(ap, format);
+  int r = vsnprintf(buf, sizeof buf, format, ap);
+  va_end(ap);
+  if (write(STDERR_FILENO, buf, r) <= 0) {
+    // We don't care, just placate the compiler.
+  }
+}
+
 // Signal handler.
 static void handler(int signum) {
+  int saved_errno = errno;
+
   switch (signum) {
     case SIGINT:
       if (++globals->sigint_count >= 3)  {
         sigprintf("\n%s caught third interrupt signal; killed.\n\n",
                   globals->options.product_name.c_str());
         if (globals->server_pid != -1) {
-          KillServerProcess(globals->server_pid, globals->options.output_base,
-                            globals->options.install_base);
+          KillServerProcess(globals->server_pid);
         }
         _exit(1);
       }
@@ -1473,6 +1496,8 @@ static void handler(int signum) {
       kill(globals->server_pid, SIGQUIT);
       break;
   }
+
+  errno = saved_errno;
 }
 
 // Constructs the command line for a server request.
@@ -2026,8 +2051,10 @@ void GrpcBlazeServer::KillRunningServer() {
   while (reader->Read(&response)) {}
 
   // Kill the server process for good measure.
-  KillServerProcess(globals->server_pid, globals->options.output_base,
-                    globals->options.install_base);
+  if (VerifyServerProcess(globals->server_pid, globals->options.output_base,
+                          globals->options.install_base)) {
+    KillServerProcess(globals->server_pid);
+  }
 
   connected_ = false;
 }
diff --git a/src/main/cpp/blaze_util.cc b/src/main/cpp/blaze_util.cc
index 7bf95cbade..a1cf14a225 100644
--- a/src/main/cpp/blaze_util.cc
+++ b/src/main/cpp/blaze_util.cc
@@ -176,7 +176,7 @@ int MakeDirectories(const string& path, mode_t mode) {
 }
 
 // Replaces 'contents' with contents of 'fd' file descriptor.
-// Returns false on error. Can be called from a signal handler.
+// Returns false on error.
 bool ReadFileDescriptor(int fd, string *content) {
   content->clear();
   char buf[4096];
@@ -193,7 +193,7 @@ bool ReadFileDescriptor(int fd, string *content) {
 }
 
 // Replaces 'content' with contents of file 'filename'.
-// Returns false on error. Can be called from a signal handler.
+// Returns false on error.
 bool ReadFile(const string &filename, string *content) {
   int fd = open(filename.c_str(), O_RDONLY);
   if (fd == -1) return false;
@@ -435,21 +435,4 @@ void ReleaseLock(BlazeLock* blaze_lock) {
   close(blaze_lock->lockfd);
 }
 
-// WARNING: any output from the blaze client may be interleaved
-// with output from the blaze server.  In --curses mode,
-// the Blaze server often erases the previous line of output.
-// So, be sure to end each such message with TWO newlines,
-// otherwise it may be erased by the next message from the
-// Blaze server.
-// Also, it's a good idea to start each message with a newline,
-// in case the Blaze server has written a partial line.
-void sigprintf(const char *format, ...) {
-  char buf[1024];
-  va_list ap;
-  va_start(ap, format);
-  int r = vsnprintf(buf, sizeof buf, format, ap);
-  va_end(ap);
-  (void) write(STDERR_FILENO, buf, r);
-}
-
 }  // namespace blaze
diff --git a/src/main/cpp/blaze_util.h b/src/main/cpp/blaze_util.h
index 89181115e8..0472ddc9ec 100644
--- a/src/main/cpp/blaze_util.h
+++ b/src/main/cpp/blaze_util.h
@@ -119,9 +119,6 @@ string ToString(const T& value) {
   return oss.str();
 }
 
-// A signal-safe version of fprintf(stderr, ...).
-void sigprintf(const char *format, ...);
-
 }  // namespace blaze
 
 #endif  // BAZEL_SRC_MAIN_CPP_BLAZE_UTIL_H_
diff --git a/src/main/cpp/blaze_util_darwin.cc b/src/main/cpp/blaze_util_darwin.cc
index 1f0acbd2cf..93645b12eb 100644
--- a/src/main/cpp/blaze_util_darwin.cc
+++ b/src/main/cpp/blaze_util_darwin.cc
@@ -186,10 +186,14 @@ string GetDefaultHostJavabase() {
 void WriteSystemSpecificProcessIdentifier(const string& server_dir) {
 }
 
-bool KillServerProcess(
+bool VerifyServerProcess(
     int pid, const string& output_base, const string& install_base) {
   // TODO(lberki): This might accidentally kill an unrelated process if the
   // server died and the PID got reused.
+  return true;
+}
+
+bool KillServerProcess(int pid) {
   killpg(pid, SIGKILL);
   return true;
 }
diff --git a/src/main/cpp/blaze_util_freebsd.cc b/src/main/cpp/blaze_util_freebsd.cc
index 4429038258..d7b7354c71 100644
--- a/src/main/cpp/blaze_util_freebsd.cc
+++ b/src/main/cpp/blaze_util_freebsd.cc
@@ -155,10 +155,14 @@ string GetDefaultHostJavabase() {
 void WriteSystemSpecificProcessIdentifier(const string& server_dir) {
 }
 
-bool KillServerProcess(
+bool VerifyServerProcess(
     int pid, const string& output_base, const string& install_base) {
   // TODO(lberki): This might accidentally kill an unrelated process if the
   // server died and the PID got reused.
+  return true;
+}
+
+bool KillServerProcess(int pid) {
   killpg(pid, SIGKILL);
   return true;
 }
diff --git a/src/main/cpp/blaze_util_linux.cc b/src/main/cpp/blaze_util_linux.cc
index bd5fc852cb..5db5cb538a 100644
--- a/src/main/cpp/blaze_util_linux.cc
+++ b/src/main/cpp/blaze_util_linux.cc
@@ -20,7 +20,6 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>  // strerror
-#include <sys/fcntl.h>
 #include <sys/socket.h>
 #include <sys/statfs.h>
 #include <sys/types.h>
@@ -168,71 +167,34 @@ string GetDefaultHostJavabase() {
   return blaze_util::Dirname(blaze_util::Dirname(javac_dir));
 }
 
-// Called from a signal handler. Therefore, we can't use our usual set of
-// helper functions for reading files, splitting strings and so on.
-static bool GetStartTime(int pid, char* output, int output_len) {
-  char statfile[128];
-  snprintf(statfile, sizeof(statfile), "/proc/%d/stat", pid);
-  int fd = open(statfile, O_RDONLY);
-  if (fd < 0) {
-    return false;
-  }
+// Called from a signal handler!
+static bool GetStartTime(const string& pid, string* start_time) {
+  string statfile = "/proc/" + pid + "/stat";
+  string statline;
 
-  // Note that this allocates 1K on any random stack the signal handler is
-  // called on
-  char statline[1024];
-  int statline_len = read(fd, statline, 1024);
-  close(fd);
-  if (statline_len < 0) {
+  if (!ReadFile(statfile, &statline)) {
     return false;
   }
 
-  // Field 22 is that start time of the process since system startup in jiffies.
-  int space_count = 0;
-  int space_21 = -1;
-  int space_22 = -1;
-
-  for (int i = 0; i < statline_len; i++) {
-    if (statline[i] == ' ') {
-      switch (++space_count) {
-        case 21:
-          space_21 = i;
-          break;
-
-        case 22:
-          space_22 = i;
-          break;
-
-        default:
-          // We don't care
-          break;
-      }
-    }
-  }
-
-  if (space_21 == -1 || space_22 == -1) {
-    // Invalid statline format
+  vector<string> stat_entries = blaze_util::Split(statline, ' ');
+  if (stat_entries.size() < 22) {
     pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-         "Format of stat file at %s is unknown", statfile);
+         "Format of stat file at %s is unknown", statfile.c_str());
   }
 
-  int jiffies_len = space_22 - space_21 - 1;
-  if (jiffies_len >= output_len) {
-    // Not enough space in output buffer (Note that we need one extra byte
-    // for the terminating NUL!)
-    return false;
-  }
-
-  strncpy(output, statline + space_21 + 1, jiffies_len);
-  output[jiffies_len] = 0;
+  // Start time since startup in jiffies. This combined with the PID should be
+  // unique.
+  *start_time = stat_entries[21];
   return true;
 }
 
 void WriteSystemSpecificProcessIdentifier(const string& server_dir) {
-  char start_time[256];
-  if (!GetStartTime(getpid(), start_time, 256)) {
+  string pid = ToString(getpid());
+
+  string start_time;
+  if (!GetStartTime(pid, &start_time)) {
     pdie(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR,
-         "Cannot get start time of process %d", getpid());
+         "Cannot get start time of process %s", pid.c_str());
   }
 
   string start_time_file = blaze_util::JoinPath(server_dir, "server.starttime");
@@ -245,13 +207,10 @@ void WriteSystemSpecificProcessIdentifier(const string& server_dir) {
 // On Linux we use a combination of PID and start time to identify the server
 // process. That is supposed to be unique unless one can start more processes
 // than there are PIDs available within a single jiffy.
-//
-// This looks complicated, but all it does is an open(), then read(), then
-// close(), all of which are safe to call from signal handlers.
-bool KillServerProcess(
+bool VerifyServerProcess(
     int pid, const string& output_base, const string& install_base) {
-  char start_time[256];
-  if (!GetStartTime(pid, start_time, sizeof(start_time))) {
+  string start_time;
+  if (!GetStartTime(ToString(pid), &start_time)) {
     // Cannot read PID file from /proc . Process died meantime, all is good. No
     // stale server is present.
     return false;
@@ -262,14 +221,12 @@ bool KillServerProcess(
       blaze_util::JoinPath(output_base, "server/server.starttime"),
       &recorded_start_time);
 
-  // start time file got deleted, but PID file didn't. This is strange.
-  // Assume that this is an old Blaze process that doesn't know how to write
-  // start time files yet.
-  if (file_present && recorded_start_time != start_time) {
-    // This is a different process.
-    return false;
-  }
+  // If start time file got deleted, but PID file didn't, assume taht this is an
+  // old Blaze process that doesn't know how to write start time files yet.
+  return !file_present || recorded_start_time == start_time;
+}
 
+bool KillServerProcess(int pid) {
   // Kill the process and make sure it's dead before proceeding.
   killpg(pid, SIGKILL);
   int check_killed_retries = 10;
diff --git a/src/main/cpp/blaze_util_mingw.cc b/src/main/cpp/blaze_util_mingw.cc
index 069ce8bb5e..4807df4f8c 100644
--- a/src/main/cpp/blaze_util_mingw.cc
+++ b/src/main/cpp/blaze_util_mingw.cc
@@ -753,8 +753,14 @@ bool CompareAbsolutePaths(const string& a, const string& b) {
   return a_real == b_real;
 }
 
-bool KillServerProcess(
+bool VerifyServerProcess(
     int pid, const string& output_base, const string& install_base) {
+  // TODO(lberki): This might accidentally kill an unrelated process if the
+  // server died and the PID got reused.
+  return true;
+}
+
+bool KillServerProcess(int pid) {
   HANDLE process = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
   if (process == NULL) {
     // Cannot find the server process. Can happen if the PID file is stale.
diff --git a/src/main/cpp/blaze_util_platform.h b/src/main/cpp/blaze_util_platform.h
index 5f8b3bfa79..3f4c880d2e 100644
--- a/src/main/cpp/blaze_util_platform.h
+++ b/src/main/cpp/blaze_util_platform.h
@@ -120,12 +120,15 @@ uint64_t AcquireLock(const string& output_base, bool batch_mode,
 // usual.
 void ReleaseLock(BlazeLock* blaze_lock);
 
-// Kills a server process based on its output base and PID. Returns true if the
-// server process was found and killed.
-// This function can be called from a signal handler!
-bool KillServerProcess(
+// Verifies whether the server process still exists. Returns true if it does.
+bool VerifyServerProcess(
     int pid, const string& output_base, const string& install_base);
 
+// Kills a server process based on its PID. Returns true if the
+// server process was found and killed. This function can be called from a
+// signal handler! Returns true if successful.
+bool KillServerProcess(int pid);
+
 // Mark path as being excluded from backups (if supported by operating system).
 void ExcludePathFromBackup(const string &path);