From 6dc59629570ae6fe1836cff3ff53912917d17af7 Mon Sep 17 00:00:00 2001 From: Fangrui Song Date: Thu, 11 Jul 2019 10:17:59 +0000 Subject: [PATCH] [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 | 10 ++++++---- llvm/lib/Support/Unix/Path.inc | 12 +++++++----- llvm/lib/Support/Windows/Path.inc | 8 ++++++-- .../llvm-objcopy/ELF/mirror-permissions-unix.test | 6 ++++++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp | 15 +++++++++++++-- llvm/unittests/Support/Path.cpp | 10 ++++++---- 6 files changed, 44 insertions(+), 17 deletions(-) diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h index 3f2c93985cc8..1bec27bddad9 100644 --- a/llvm/include/llvm/Support/FileSystem.h +++ b/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. /// diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc index 58c15023bab1..e80880c6b3cb 100644 --- a/llvm/lib/Support/Unix/Path.inc +++ b/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(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/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc index 045d0b9ed58e..5704930aeecc 100644 --- a/llvm/lib/Support/Windows/Path.inc +++ b/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 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/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test index 0c1f6b985a7a..ce6e79b1f2bc 100644 --- a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test +++ b/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 diff --git a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp index 2b0fef117f89..e9372176e43b 100644 --- a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp +++ b/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(Stat.permissions() & + ~sys::fs::getUmask()))) +#else + if (auto EC = sys::fs::setPermissions( + FD, static_cast(Stat.permissions() & + ~sys::fs::getUmask()))) +#endif + return createFileError(Filename, EC); if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD)) return createFileError(Filename, EC); diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp index a70ca89e3857..ccd72d7f1769 100644 --- a/llvm/unittests/Support/Path.cpp +++ b/llvm/unittests/Support/Path.cpp @@ -1550,19 +1550,20 @@ TEST_F(FileSystemTest, RespectUmask) { fs::perms AllRWE = static_cast(0777); - ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE /*RespectUmask=false*/)); + ASSERT_NO_ERROR(fs::setPermissions(TempPath, AllRWE)); ErrorOr 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(AllRWE & ~fs::getUmask()))); Perms = fs::getPermissions(TempPath); ASSERT_TRUE(!!Perms); EXPECT_EQ(Perms.get(), static_cast(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(AllRWE & ~fs::getUmask()))); Perms = fs::getPermissions(TempPath); ASSERT_TRUE(!!Perms); EXPECT_EQ(Perms.get(), static_cast(0720))