[Support] Add fs::getUmask() function and change fs::setPermissions

[abrachet]

Summary

This patch changes fs::setPermissions to optionally set permissions while respecting the umask. It also adds the function fs::getUmask() which returns the current umask.

[abrachet]

This change is not 100% necessary it doesn't need to be in lib/Support, but I am planning on using it for D62718. I figured I would test the waters on this one. Also, fs::getUmask() isn't necessary to have, I think that the change to fs::setPermissions would be enough, but I figured I would add it and remove later than the other way around.

llvm/lib/Support/Windows/Path.inc

741 ↗

(On Diff #205720)

I don't know if unnamed parameters are allowed per the style guide, it isn't used by the Windows version of this function, this was the thinking behind this.

[abrachet]

Updated doxygen comments for fs::setPermissions to explain new RespectUmask parameter.

[jhenderson]

I've added a couple of other reviewers who have made recent changes to the Path files.

llvm/lib/Support/Unix/Path.inc

695–698 ↗

(On Diff #205723)

It's not your fault, but this dance seems ridiculous.

702 ↗

(On Diff #205723)

clang-format?

709 ↗

(On Diff #205723)

Remove double-blank line.

llvm/lib/Support/Windows/Path.inc

741 ↗

(On Diff #205720)

clang-format.

I think this is fine.

llvm/unittests/Support/Path.cpp

1541 ↗

(On Diff #205723)

You're capitalization of test names is inconsistent. One starts with a leading caps, and the other doesn't.

1555 ↗

(On Diff #205723)

I think you also need a test case for explicit false for respectUmask.

[abrachet]

Added test case and changed formatting.

[abrachet]

llvm/lib/Support/Unix/Path.inc

695–698 ↗

(On Diff #205723)

Agreed, there are quite a few get/set syscalls, not sure why this needed to be the case for umask.

702 ↗

(On Diff #205723)

Yup sorry about that I should have looked over git clang-formats changes. It did not format the .inc files. This should be proper formatting now, I just copied how it is in the header file which clang-format did.

llvm/unittests/Support/Path.cpp

1541 ↗

(On Diff #205723)

I think this is consistent with the rest of the file. getUmask is capitalized as such because it is testing a specific function, while RespectUmask is testing behavior. is_local test versus the ReadWriteFileCanReadOrWrite test.

[jhenderson]

LGTM. Maybe give it a couple of days before committing for a chance for others to say something.

[rupprecht]

llvm/include/llvm/Support/FileSystem.h

654–656 ↗

(On Diff #205860)

Also note that it is not threadsafe

llvm/lib/Support/Unix/Path.inc

695 ↗

(On Diff #205860)

nit: ret set -> reset

[abrachet]

Fixed typo in comment, added note that getUmask() is not thread safe.

llvm/trunk/include/llvm/Support/FileSystem.h

 /// @returns errc::success if result has been successfully set, otherwise a
 ///          platform-specific error_code.
 std::error_code status(const Twine &path, file_status &result,
                        bool follow = true);
 
 /// A version for when a file descriptor is already available.
 std::error_code status(int FD, file_status &Result);
 
+/// Get file creation mode mask of the process.
+///
+/// @returns Mask reported by umask(2)
+/// @note There is no umask on Windows. This function returns 0 always
+///       on Windows. This function does not return an error_code because
+///       umask(2) never fails. It is not thread safe.
+unsigned getUmask();
+
 /// Set file permissions.
 ///
 /// @param Path File to set permissions on.
 /// @param Permissions New file permissions.
+/// @param RespectUmask If true then Permissions will be changed to respect the
+///        umask of the current process.
 /// @returns errc::success if the permissions were successfully set, otherwise
 ///          a platform-specific error_code.
 /// @note On Windows, all permissions except *_write are ignored. Using any of
 ///       owner_write, group_write, or all_write will make the file writable.
 ///       Otherwise, the file will be marked as read-only.
-std::error_code setPermissions(const Twine &Path, perms Permissions);
+std::error_code setPermissions(const Twine &Path, perms Permissions,
+                               bool RespectUmask = false);
 
 /// Get file permissions.
 ///
 /// @param Path File to get permissions from.
 /// @returns the permissions if they were successfully retrieved, otherwise a
 ///          platform-specific error_code.
 /// @note On Windows, if the file does not have the FILE_ATTRIBUTE_READONLY
 ///       attribute, all_all will be returned. Otherwise, all_read | all_exe

llvm/trunk/lib/Support/Unix/Path.inc

 }
 
 std::error_code status(int FD, file_status &Result) {
   struct stat Status;
   int StatRet = ::fstat(FD, &Status);
   return fillStatus(StatRet, Status, Result);
 }
 
-std::error_code setPermissions(const Twine &Path, perms Permissions) {
+unsigned getUmask() {
+  // Chose arbitary new mask and reset the umask to the old mask.
+  // umask(2) never fails so ignore the return of the second call.
+  unsigned Mask = ::umask(0);
+  (void) ::umask(Mask);
+  return Mask;
+}
+
+std::error_code setPermissions(const Twine &Path, perms Permissions,
+                               bool RespectUmask) {
   SmallString<128> PathStorage;
   StringRef P = Path.toNullTerminatedStringRef(PathStorage);
 
+  if (RespectUmask)
+    Permissions = static_cast<perms>(Permissions & ~getUmask());
+
   if (::chmod(P.begin(), Permissions))
     return std::error_code(errno, std::generic_category());
   return std::error_code();
 }
 
 std::error_code setLastAccessAndModificationTime(int FD, TimePoint<> AccessTime,
                                                  TimePoint<> ModificationTime) {
 #if defined(HAVE_FUTIMENS)

llvm/trunk/lib/Support/Windows/Path.inc

std::error_code status(const Twine &path, file_status &result, bool Follow) {
   return getStatus(h, result);
 }
 
 std::error_code status(int FD, file_status &Result) {
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   return getStatus(FileHandle, Result);
 }
 
-std::error_code setPermissions(const Twine &Path, perms Permissions) {
+unsigned getUmask() {
+  return 0;
+}
+
+std::error_code setPermissions(const Twine &Path, perms Permissions,
+                               bool /*RespectUmask*/) {
   SmallVector<wchar_t, 128> PathUTF16;
   if (std::error_code EC = widenPath(Path, PathUTF16))
     return EC;
 
   DWORD Attributes = ::GetFileAttributesW(PathUTF16.begin());
   if (Attributes == INVALID_FILE_ATTRIBUTES)
     return mapWindowsError(GetLastError());
 

llvm/trunk/unittests/Support/Path.cpp

TEST_F(FileSystemTest, is_local) {
   EXPECT_EQ(TempFileIsLocal, fs::is_local(FD));
   ::close(FD);
 
   // Expect that the file and its parent directory are equally local or equally
   // remote.
   EXPECT_EQ(TestDirectoryIsLocal, TempFileIsLocal);
 }
 
+TEST_F(FileSystemTest, getUmask) {
+#ifdef _WIN32
+  EXPECT_EQ(fs::getUmask(), 0U) << "Should always be 0 on Windows.";
+#else
+  unsigned OldMask = ::umask(0022);
+  unsigned CurrentMask = fs::getUmask();
+  EXPECT_EQ(CurrentMask, 0022U)
+      << "getUmask() didn't return previously set umask()";
+  EXPECT_EQ(::umask(OldMask), 0022) << "getUmask() may have changed umask()";
+#endif
+}
+
+TEST_F(FileSystemTest, RespectUmask) {
+#ifndef _WIN32
+  unsigned OldMask = ::umask(0022);
+
+  int FD;
+  SmallString<128> TempPath;
+  ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD, TempPath));
+
+  fs::perms AllRWE = static_cast<fs::perms>(0777);
+
+  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE /*RespectUmask=false*/));
+
+  ErrorOr<fs::perms> Perms = fs::getPermissions(TempPath);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(Perms.get(), AllRWE) << "Should have ignored umask by default";
+
+  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE, /*RespectUmask=*/false));
+
+  ErrorOr<fs::perms> Perms = fs::getPermissions(TempPath);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(Perms.get(), AllRWE) << "Should have ignored umask";
+
+  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE, /*RespectUmask=*/true));
+  Perms = fs::getPermissions(TempPath);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(Perms.get(), static_cast<fs::perms>(0755))
+      << "Did not respect umask";
+
+  (void)::umask(0057);
+
+  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE, /*RespectUmask=*/true));
+  Perms = fs::getPermissions(TempPath);
+  ASSERT_TRUE(!!Perms);
+  EXPECT_EQ(Perms.get(), static_cast<fs::perms>(0720))
+      << "Did not respect umask";
+
+  (void)::umask(OldMask);
+  (void)::close(FD);
+#endif
+}
+
 TEST_F(FileSystemTest, set_current_path) {
   SmallString<128> path;
 
   ASSERT_NO_ERROR(fs::current_path(path));
   ASSERT_NE(TestDirectory, path);
 
   struct RestorePath {
     SmallString<128> path;