[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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@365753 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/Support/FileSystem.h b/include/llvm/Support/FileSystem.h
index 3f2c93985cc8a876095b215826c7f75f417acdbd..1bec27bddad9ec39bc8bdecd03b00530fbb10594 100644 (file)
--- a/include/llvm/Support/FileSystem.h
+++ b/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.
 ///

diff --git a/lib/Support/Unix/Path.inc b/lib/Support/Unix/Path.inc
index 58c15023bab1549ebc30f8cd288d40a018863145..e80880c6b3cb8bd0f2d7f9830eb9fc26acc7a723 100644 (file)
--- a/lib/Support/Unix/Path.inc
+++ b/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)

diff --git a/lib/Support/Windows/Path.inc b/lib/Support/Windows/Path.inc
index 045d0b9ed58e1f7b8aeaf4828a6de2e596b38955..5704930aeeccb3dc73a310b6bd0788cba7fb2715 100644 (file)
--- a/lib/Support/Windows/Path.inc
+++ b/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);

diff --git a/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test b/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
index 0c1f6b985a7a901ac37f3bf162677223431d8ac3..ce6e79b1f2bca74dfb40b8cb47d94e7ab6fac43c 100644 (file)
--- a/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
+++ b/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

diff --git a/tools/llvm-objcopy/llvm-objcopy.cpp b/tools/llvm-objcopy/llvm-objcopy.cpp
index 2b0fef117f8930e07040c2d522bc5354dec15c0d..e9372176e43b2356f4f8b83f74ed360c133f2f76 100644 (file)
--- a/tools/llvm-objcopy/llvm-objcopy.cpp
+++ b/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);

diff --git a/unittests/Support/Path.cpp b/unittests/Support/Path.cpp
index a70ca89e385768e557cfa987262710abac6d32b1..ccd72d7f176975c858ae34529c2e977622535575 100644 (file)
--- a/unittests/Support/Path.cpp
+++ b/unittests/Support/Path.cpp
@@ -1550,19 +1550,20 @@ 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))
@@ -1570,7 +1571,8 @@ TEST_F(FileSystemTest, RespectUmask) {
 
   (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))