Windows,Bazel client: check embedded tools faster

The Bazel client on Windows is now 50% faster to
check the embedded tools than it was before.

Results:
- Linux: 20 ms -> 6 ms
- Windows: 294 ms -> 133 ms

Measurements were done with n=10 runs and a hot
server, using blaze::GetMillisecondsMonotonic().

Previously the client performed the same tasks
multiple times while trying to determine if a path
was a good extracted binary. (E.g. converted the
path to Windows format multiple times, checked if
it was a directory twice, opened the path twice.)

Now the client performes these tasks only once,
e.g. it converts path once and stats only once.

See https://github.com/bazelbuild/bazel/issues/5444

Closes #5445.

PiperOrigin-RevId: 201913758

4 files changed, 51 insertions, 60 deletions

diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index a8987eb2b2..5073fe513c 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -1052,7 +1052,7 @@ static void ExtractData(const string &self_path) {
   } else {
     if (!blaze_util::IsDirectory(globals->options->install_base)) {
       BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
-          << "Install base directory '" << globals->options->install_base
+          << "install base directory '" << globals->options->install_base
           << "' could not be created. It exists but is not a directory.";
     }
 
@@ -1062,31 +1062,11 @@ static void ExtractData(const string &self_path) {
         globals->options->install_base, "_embedded_binaries");
     for (const auto &it : globals->extracted_binaries) {
       string path = blaze_util::JoinPath(real_install_dir, it);
-      // Check that the file exists and is readable.
-      if (blaze_util::IsDirectory(path)) {
-        continue;
-      }
-      if (!blaze_util::CanReadFile(path)) {
+      if (!mtime->IsUntampered(path)) {
         BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
             << "corrupt installation: file '" << path
-            << "' missing. Please remove '" << globals->options->install_base
-            << "' and try again.";
-      }
-      // Check that the timestamp is in the future. A past timestamp would
-      // indicate that the file has been tampered with.
-      // See ActuallyExtractData().
-      bool is_in_future = false;
-      if (!mtime.get()->GetIfInDistantFuture(path, &is_in_future)) {
-        BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
-            << "Error: could not retrieve mtime of file '" << path
-            << "'. Please remove '" << globals->options->install_base
-            << "' and try again.";
-      }
-      if (!is_in_future) {
-        BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
-            << "Error: corrupt installation: file '" << path
-            << "' modified.  Please remove '" << globals->options->install_base
-            << "' and try again.";
+            << "' is missing or modified.  Please remove '"
+            << globals->options->install_base << "' and try again.";
       }
     }
   }
diff --git a/src/main/cpp/util/file_platform.h b/src/main/cpp/util/file_platform.h
index 861ca9e997..d7538fd86c 100644
--- a/src/main/cpp/util/file_platform.h
+++ b/src/main/cpp/util/file_platform.h
@@ -31,10 +31,15 @@ class IFileMtime {
  public:
   virtual ~IFileMtime() {}
 
-  // Queries the mtime of `path` to see whether it's in the distant future.
-  // Returns true if querying succeeded and stores the result in `result`.
-  // Returns false if querying failed.
-  virtual bool GetIfInDistantFuture(const std::string &path, bool *result) = 0;
+  // Checks if `path` is a file/directory in the embedded tools directory that
+  // was not tampered with.
+  // Returns true if `path` is a directory or directory symlink, or if `path` is
+  // a file with an mtime in the distant future.
+  // Returns false otherwise, or if querying the information failed.
+  // TODO(laszlocsomor): move this function, and with it the whole IFileMtime
+  // class into blaze_util_<platform>.cc, because it is Bazel-specific logic,
+  // not generic file-handling logic.
+  virtual bool IsUntampered(const std::string &path) = 0;
 
   // Sets the mtime of file under `path` to the current time.
   // Returns true if the mtime was changed successfully.
diff --git a/src/main/cpp/util/file_posix.cc b/src/main/cpp/util/file_posix.cc
index df05b54a45..f9e96451dc 100644
--- a/src/main/cpp/util/file_posix.cc
+++ b/src/main/cpp/util/file_posix.cc
@@ -329,7 +329,7 @@ class PosixFileMtime : public IFileMtime {
       : near_future_(GetFuture(9)),
         distant_future_({GetFuture(10), GetFuture(10)}) {}
 
-  bool GetIfInDistantFuture(const string &path, bool *result) override;
+  bool IsUntampered(const string &path) override;
   bool SetToNow(const string &path) override;
   bool SetToDistantFuture(const string &path) override;
 
@@ -344,18 +344,18 @@ class PosixFileMtime : public IFileMtime {
   static time_t GetFuture(unsigned int years);
 };
 
-bool PosixFileMtime::GetIfInDistantFuture(const string &path, bool *result) {
+bool PosixFileMtime::IsUntampered(const string &path) {
   struct stat buf;
   if (stat(path.c_str(), &buf)) {
     return false;
   }
+
   // Compare the mtime with `near_future_`, not with `GetNow()` or
   // `distant_future_`.
   // This way we don't need to call GetNow() every time we want to compare and
   // we also don't need to worry about potentially unreliable time equality
   // check (in case it uses floats or something crazy).
-  *result = (buf.st_mtime > near_future_);
-  return true;
+  return S_ISDIR(buf.st_mode) || (buf.st_mtime > near_future_);
 }
 
 bool PosixFileMtime::SetToNow(const string &path) {
diff --git a/src/main/cpp/util/file_windows.cc b/src/main/cpp/util/file_windows.cc
index 8a18403c91..d81ada1b14 100644
--- a/src/main/cpp/util/file_windows.cc
+++ b/src/main/cpp/util/file_windows.cc
@@ -109,7 +109,7 @@ class WindowsFileMtime : public IFileMtime {
   WindowsFileMtime()
       : near_future_(GetFuture(9)), distant_future_(GetFuture(10)) {}
 
-  bool GetIfInDistantFuture(const string& path, bool* result) override;
+  bool IsUntampered(const string& path) override;
   bool SetToNow(const string& path) override;
   bool SetToDistantFuture(const string& path) override;
 
@@ -124,53 +124,59 @@ class WindowsFileMtime : public IFileMtime {
   static bool Set(const string& path, FILETIME time);
 };
 
-bool WindowsFileMtime::GetIfInDistantFuture(const string& path, bool* result) {
-  if (path.empty()) {
+bool WindowsFileMtime::IsUntampered(const string& path) {
+  if (path.empty() || IsDevNull(path.c_str())) {
     return false;
   }
-  if (IsDevNull(path.c_str())) {
-    *result = false;
-    return true;
-  }
+
   wstring wpath;
   string error;
   if (!AsAbsoluteWindowsPath(path, &wpath, &error)) {
     BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
-        << "WindowsFileMtime::GetIfInDistantFuture(" << path
+        << "WindowsFileMtime::IsUntampered(" << path
         << "): AsAbsoluteWindowsPath failed: " << error;
   }
 
-  AutoHandle handle(::CreateFileW(
+  // Get attributes, to check if the file exists. (It may still be a dangling
+  // junction.)
+  DWORD attrs = GetFileAttributesW(wpath.c_str());
+  if (attrs == INVALID_FILE_ATTRIBUTES) {
+    return false;
+  }
+
+  bool is_directory = attrs & FILE_ATTRIBUTE_DIRECTORY;
+  AutoHandle handle(CreateFileW(
       /* lpFileName */ wpath.c_str(),
       /* dwDesiredAccess */ GENERIC_READ,
       /* dwShareMode */ FILE_SHARE_READ,
       /* lpSecurityAttributes */ NULL,
       /* dwCreationDisposition */ OPEN_EXISTING,
       /* dwFlagsAndAttributes */
-      IsDirectoryW(wpath)
-          ? (FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS)
-          : FILE_ATTRIBUTE_NORMAL,
+      // Per CreateFile's documentation on MSDN, opening directories requires
+      // the FILE_FLAG_BACKUP_SEMANTICS flag.
+      is_directory ? FILE_FLAG_BACKUP_SEMANTICS : FILE_ATTRIBUTE_NORMAL,
       /* hTemplateFile */ NULL));
+
   if (!handle.IsValid()) {
     return false;
   }
-  FILETIME mtime;
-  if (!::GetFileTime(
-          /* hFile */ handle,
-          /* lpCreationTime */ NULL,
-          /* lpLastAccessTime */ NULL,
-          /* lpLastWriteTime */ &mtime)) {
-    return false;
-  }
 
-  // Compare the mtime with `near_future_`, not with `GetNow()` or
-  // `distant_future_`.
-  // This way we don't need to call GetNow() every time we want to compare (and
-  // thus convert a SYSTEMTIME to FILETIME), and we also don't need to worry
-  // about potentially unreliable FILETIME equality check (in case it uses
-  // floats or something crazy).
-  *result = CompareFileTime(&near_future_, &mtime) == -1;
-  return true;
+  if (is_directory) {
+    return true;
+  } else {
+    BY_HANDLE_FILE_INFORMATION info;
+    if (!GetFileInformationByHandle(handle, &info)) {
+      return false;
+    }
+
+    // Compare the mtime with `near_future_`, not with `GetNow()` or
+    // `distant_future_`.
+    // This way we don't need to call GetNow() every time we want to compare
+    // (and thus convert a SYSTEMTIME to FILETIME), and we also don't need to
+    // worry about potentially unreliable FILETIME equality check (in case it
+    // uses floats or something crazy).
+    return CompareFileTime(&near_future_, &info.ftLastWriteTime) == -1;
+  }
 }
 
 bool WindowsFileMtime::SetToNow(const string& path) {