[llvm-objcopy] Don't change permissions of non-regular output files

There is currently an EPERM error when a regular user executes `llvm-objcopy a.o /dev/null`.
Worse, root can even change the mode bits of /dev/null.

Fix it by checking if the output file is special.

A new overload of llvm::sys::fs::setPermissions with FD as the parameter
is added. Users should provide `perm & ~umask` as the parameter if they
intend to respect umask.

The existing overload of llvm::sys::fs::setPermissions may be deleted if
we can find an implementation of fchmod() on Windows. fchmod() is
usually better than chmod() because it saves syscalls and can avoid race
condition.

Reviewed By: jakehehrlich, jhenderson

Differential Revision: https://reviews.llvm.org/D64236

llvm-svn: 365753

llvm/include/llvm/Support/FileSystem.h

@@ -665,15 +665,17 @@ unsigned getUmask();
 ///
 /// @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,
-                               bool RespectUmask = false);
+std::error_code setPermissions(const Twine &Path, perms Permissions);
+
+/// Vesion of setPermissions accepting a file descriptor.
+/// TODO Delete the path based overload once we implement the FD based overload
+/// on Windows.
+std::error_code setPermissions(int FD, perms Permissions);
 
 /// Get file permissions.
 ///

llvm/lib/Support/Unix/Path.inc

@@ -702,19 +702,21 @@ unsigned getUmask() {
   return Mask;
 }
 
-std::error_code setPermissions(const Twine &Path, perms Permissions,
-                               bool RespectUmask) {
+std::error_code setPermissions(const Twine &Path, perms Permissions) {
   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 setPermissions(int FD, perms Permissions) {
+  if (::fchmod(FD, 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/lib/Support/Windows/Path.inc

@@ -742,8 +742,7 @@ unsigned getUmask() {
   return 0;
 }
 
-std::error_code setPermissions(const Twine &Path, perms Permissions,
-                               bool /*RespectUmask*/) {
+std::error_code setPermissions(const Twine &Path, perms Permissions) {
   SmallVector<wchar_t, 128> PathUTF16;
   if (std::error_code EC = widenPath(Path, PathUTF16))
     return EC;
@@ -774,6 +773,11 @@ std::error_code setPermissions(const Twine &Path, perms Permissions,
   return std::error_code();
 }
 
+std::error_code setPermissions(int FD, perms Permissions) {
+  // FIXME Not implemented.
+  return std::make_error_code(std::errc::not_supported);
+}
+
 std::error_code setLastAccessAndModificationTime(int FD, TimePoint<> AccessTime,
                                                  TimePoint<> ModificationTime) {
   FILETIME AccessFT = toFILETIME(AccessTime);

llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test

@@ -36,6 +36,12 @@
 # RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
 # RUN: cmp %t1.perms %t.0640
 
+## Don't set the permission of a character special file, otherwise there will
+## be an EPERM error (or worse: root may change the permission).
+# RUN: ls -l /dev/null | cut -f 1 -d ' ' > %tnull.perms
+# RUN: llvm-objcopy %t /dev/null
+# RUN: ls -l /dev/null | cut -f 1 -d ' ' | diff - %tnull.perms
+
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

@@ -222,9 +222,20 @@ static Error restoreStatOnFile(StringRef Filename,
             FD, Stat.getLastAccessedTime(), Stat.getLastModificationTime()))
       return createFileError(Filename, EC);
 
-  if (auto EC = sys::fs::setPermissions(Filename, Stat.permissions(),
-                                        /*respectUmask=*/true))
+  sys::fs::file_status OStat;
+  if (std::error_code EC = sys::fs::status(FD, OStat))
     return createFileError(Filename, EC);
+  if (OStat.type() == sys::fs::file_type::regular_file)
+#ifdef _WIN32
+    if (auto EC = sys::fs::setPermissions(
+            Filename, static_cast<sys::fs::perms>(Stat.permissions() &
+                                                  ~sys::fs::getUmask())))
+#else
+    if (auto EC = sys::fs::setPermissions(
+            FD, static_cast<sys::fs::perms>(Stat.permissions() &
+                                            ~sys::fs::getUmask())))
+#endif
+      return createFileError(Filename, EC);
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(Filename, EC);

llvm/unittests/Support/Path.cpp

@@ -1550,27 +1550,29 @@ TEST_F(FileSystemTest, RespectUmask) {
 
   fs::perms AllRWE = static_cast<fs::perms>(0777);
 
-  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE /*RespectUmask=false*/));
+  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE));
 
   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));
+  ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE));
 
   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));
+  ASSERT_NO_ERROR(
+      fs::setPermissions(FD, static_cast<fs::perms>(AllRWE & ~fs::getUmask())));
   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));
+  ASSERT_NO_ERROR(
+      fs::setPermissions(FD, static_cast<fs::perms>(AllRWE & ~fs::getUmask())));
   Perms = fs::getPermissions(TempPath);
   ASSERT_TRUE(!!Perms);
   EXPECT_EQ(Perms.get(), static_cast<fs::perms>(0720))