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

7 files changed, 91 insertions, 78 deletions

diff --git a/src/java_tools/junitrunner/java/com/google/testing/coverage/BUILD b/src/java_tools/junitrunner/java/com/google/testing/coverage/BUILD
index 031793165f..a5055017e0 100644
--- a/src/java_tools/junitrunner/java/com/google/testing/coverage/BUILD
+++ b/src/java_tools/junitrunner/java/com/google/testing/coverage/BUILD
@@ -70,7 +70,7 @@ genrule(
         "rm -fr \"$${JARJAR}\"",
     ]),
     tags = ["manual"],
-    toolchains = ["@bazel_tools//tools/jdk:current_java_runtime"],
+    toolchains = ["@bazel_tools//tools/jdk:current_host_java_runtime"],
     tools = [
         "//src/java_tools/singlejar:SingleJar_deploy.jar",
         "//third_party/jarjar:jarjar_bin_deploy.jar",
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) {
diff --git a/src/test/cpp/util/file_test.cc b/src/test/cpp/util/file_test.cc
index 9683d9dcbe..a7eb9c7614 100644
--- a/src/test/cpp/util/file_test.cc
+++ b/src/test/cpp/util/file_test.cc
@@ -148,37 +148,31 @@ TEST(FileTest, TestMtimeHandling) {
   string tempdir(tempdir_cstr);
 
   std::unique_ptr<IFileMtime> mtime(CreateFileMtime());
-  bool actual = false;
-  ASSERT_TRUE(mtime->GetIfInDistantFuture(tempdir, &actual));
-  ASSERT_FALSE(actual);
-
+  // Assert that a directory is always untampered with. (We do
+  // not care about directories' mtimes.)
+  ASSERT_TRUE(mtime->IsUntampered(tempdir));
   // Create a new file, assert its mtime is not in the future.
   string file(JoinPath(tempdir, "foo.txt"));
   ASSERT_TRUE(WriteFile("hello", 5, file));
-  ASSERT_TRUE(mtime->GetIfInDistantFuture(file, &actual));
-  ASSERT_FALSE(actual);
+  ASSERT_FALSE(mtime->IsUntampered(file));
   // Set the file's mtime to the future, assert that it's so.
   ASSERT_TRUE(mtime->SetToDistantFuture(file));
-  ASSERT_TRUE(mtime->GetIfInDistantFuture(file, &actual));
-  ASSERT_TRUE(actual);
-  // Overwrite the file, resetting its mtime, assert that GetIfInDistantFuture
-  // notices.
+  ASSERT_TRUE(mtime->IsUntampered(file));
+  // Overwrite the file, resetting its mtime, assert that
+  // IsUntampered notices.
   ASSERT_TRUE(WriteFile("world", 5, file));
-  ASSERT_TRUE(mtime->GetIfInDistantFuture(file, &actual));
-  ASSERT_FALSE(actual);
+  ASSERT_FALSE(mtime->IsUntampered(file));
   // Set it to the future again so we can reset it using SetToNow.
   ASSERT_TRUE(mtime->SetToDistantFuture(file));
-  ASSERT_TRUE(mtime->GetIfInDistantFuture(file, &actual));
-  ASSERT_TRUE(actual);
+  ASSERT_TRUE(mtime->IsUntampered(file));
   // Assert that SetToNow resets the timestamp.
   ASSERT_TRUE(mtime->SetToNow(file));
-  ASSERT_TRUE(mtime->GetIfInDistantFuture(file, &actual));
-  ASSERT_FALSE(actual);
+  ASSERT_FALSE(mtime->IsUntampered(file));
   // Delete the file and assert that we can no longer set or query its mtime.
   ASSERT_TRUE(UnlinkPath(file));
   ASSERT_FALSE(mtime->SetToNow(file));
   ASSERT_FALSE(mtime->SetToDistantFuture(file));
-  ASSERT_FALSE(mtime->GetIfInDistantFuture(file, &actual));
+  ASSERT_FALSE(mtime->IsUntampered(file));
 }
 
 TEST(FileTest, TestRenameDirectory) {
diff --git a/src/test/cpp/util/file_windows_test.cc b/src/test/cpp/util/file_windows_test.cc
index 7bef9cf593..c048e07950 100644
--- a/src/test/cpp/util/file_windows_test.cc
+++ b/src/test/cpp/util/file_windows_test.cc
@@ -34,6 +34,8 @@
 #error("This test should only be run on Windows")
 #endif  // !defined(_WIN32) && !defined(__CYGWIN__)
 
+#define TOSTRING(x) #x
+
 namespace blaze_util {
 
 using bazel::windows::CreateJunction;
@@ -297,4 +299,30 @@ TEST_F(FileWindowsTest, TestMakeCanonical) {
   ASSERT_EQ(dircanon, symcanon);
 }
 
+TEST_F(FileWindowsTest, TestMtimeHandling) {
+  const char* tempdir_cstr = getenv("TEST_TMPDIR");
+  ASSERT_NE(tempdir_cstr, nullptr);
+  ASSERT_NE(tempdir_cstr[0], 0);
+  string tempdir(tempdir_cstr);
+
+  string target(JoinPath(tempdir, "target" TOSTRING(__LINE__)));
+  wstring wtarget;
+  EXPECT_TRUE(AsWindowsPath(target, &wtarget, nullptr));
+  EXPECT_TRUE(CreateDirectoryW(wtarget.c_str(), NULL));
+
+  std::unique_ptr<IFileMtime> mtime(CreateFileMtime());
+  // Assert that a directory is always a good embedded binary. (We do not care
+  // about directories' mtimes.)
+  ASSERT_TRUE(mtime.get()->IsUntampered(target));
+  // Assert that junctions whose target exists are "good" embedded binaries.
+  string sym(JoinPath(tempdir, "junc" TOSTRING(__LINE__)));
+  CREATE_JUNCTION(sym, target);
+  ASSERT_TRUE(mtime.get()->IsUntampered(sym));
+  // Assert that checking fails for non-existent directories and dangling
+  // junctions.
+  EXPECT_TRUE(RemoveDirectoryW(wtarget.c_str()));
+  ASSERT_FALSE(mtime.get()->IsUntampered(target));
+  ASSERT_FALSE(mtime.get()->IsUntampered(sym));
+}
+
 }  // namespace blaze_util