commit d754448ff8de03a5360b4edd82eca22c3d640ea3
author michajlo <michajlo@google.com> Fri Mar 27 14:30:08 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Fri Mar 27 14:31:52 2020 -0700
tree 3cd67cb21136c4a03e3159a20bbc61d17a4ce187
parent 3d22faab71e2e55c83d90fd28ede163a9db703d7

Don't clobber install_base's parent's permissions

Previously we'd unconditionally try to set dirname(install_base)'s mode to
0777, then again set it to umask.

TESTED: Confirmed --install_base=/tmp/foo worked, and $unwritabledir/baz failed
PiperOrigin-RevId: 303407664

src/main/cpp/util/file_posix.cc

diff --git a/src/main/cpp/util/file_posix.cc b/src/main/cpp/util/file_posix.cc
index 4a636e8..e21e354 100644
--- a/src/main/cpp/util/file_posix.cc
+++ b/src/main/cpp/util/file_posix.cc
@@ -128,8 +128,8 @@
 string CreateTempDir(const std::string &prefix) {
   std::string parent = Dirname(prefix);
   // Need parent to exist first.
-  // TODO(b/150220877): Avoid trying to recreate a dir that already exists.
-  if (!blaze_util::MakeDirectories(parent, 0777)) {
+  if (!blaze_util::PathExists(parent) &&
+      !blaze_util::MakeDirectories(parent, 0777)) {
     BAZEL_DIE(blaze_exit_code::INTERNAL_ERROR)
         << "couldn't create '" << parent << "': "
         << blaze_util::GetLastErrorString();
@@ -139,8 +139,8 @@
   if (mkdtemp(&result[0]) == nullptr) {
     std::string err = GetLastErrorString();
     BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
-        << "could not create temporary directory to extract install base"
-        << " (" << err << ")";
+        << "could not create temporary directory under " << parent
+        << " to extract install base into (" << err << ")";
   }
 
   // There's no better way to get the current umask than to set and reset it.

src/test/cpp/util/file_posix_test.cc

diff --git a/src/test/cpp/util/file_posix_test.cc b/src/test/cpp/util/file_posix_test.cc
index 1668791..f485842 100644
--- a/src/test/cpp/util/file_posix_test.cc
+++ b/src/test/cpp/util/file_posix_test.cc
@@ -364,4 +364,21 @@
   EXPECT_TRUE(PathExists(file_symlink_target));
 }
 
+TEST(FileTest, TestCreateTempDirDoesntClobberParentPerms) {
+  const char* tempdir_cstr = getenv("TEST_TMPDIR");
+  ASSERT_NE(tempdir_cstr, nullptr);
+  string tempdir(tempdir_cstr);
+  ASSERT_TRUE(PathExists(tempdir));
+
+  string existing_parent_dir(JoinPath(tempdir, "existing"));
+  ASSERT_TRUE(MakeDirectories(existing_parent_dir, 0700));
+  string prefix(JoinPath(existing_parent_dir, "mytmp"));
+  string result(CreateTempDir(prefix));
+  ASSERT_EQ(0, result.find(prefix));
+  EXPECT_TRUE(PathExists(result));
+  struct stat filestat = {};
+  ASSERT_EQ(0, stat(existing_parent_dir.c_str(), &filestat));
+  ASSERT_EQ(mode_t(0700), filestat.st_mode & 0777);
+}
+
 }  // namespace blaze_util