[llvm] r365753 - [llvm-objcopy] Don't change permissions of non-regular output files
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 11 03:18:00 PDT 2019
Author: maskray
Date: Thu Jul 11 03:17:59 2019
New Revision: 365753
URL: http://llvm.org/viewvc/llvm-project?rev=365753&view=rev
Log:
[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
Modified:
llvm/trunk/include/llvm/Support/FileSystem.h
llvm/trunk/lib/Support/Unix/Path.inc
llvm/trunk/lib/Support/Windows/Path.inc
llvm/trunk/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp
llvm/trunk/unittests/Support/Path.cpp
Modified: llvm/trunk/include/llvm/Support/FileSystem.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=365753&r1=365752&r2=365753&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/FileSystem.h (original)
+++ llvm/trunk/include/llvm/Support/FileSystem.h Thu Jul 11 03:17:59 2019
@@ -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.
///
Modified: llvm/trunk/lib/Support/Unix/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Unix/Path.inc?rev=365753&r1=365752&r2=365753&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Unix/Path.inc (original)
+++ llvm/trunk/lib/Support/Unix/Path.inc Thu Jul 11 03:17:59 2019
@@ -702,18 +702,20 @@ 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) {
Modified: llvm/trunk/lib/Support/Windows/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?rev=365753&r1=365752&r2=365753&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Windows/Path.inc (original)
+++ llvm/trunk/lib/Support/Windows/Path.inc Thu Jul 11 03:17:59 2019
@@ -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 Twi
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);
Modified: llvm/trunk/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test?rev=365753&r1=365752&r2=365753&view=diff
==============================================================================
--- llvm/trunk/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test (original)
+++ llvm/trunk/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test Thu Jul 11 03:17:59 2019
@@ -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
Modified: llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp?rev=365753&r1=365752&r2=365753&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp (original)
+++ llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp Thu Jul 11 03:17:59 2019
@@ -222,9 +222,20 @@ static Error restoreStatOnFile(StringRef
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);
Modified: llvm/trunk/unittests/Support/Path.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/Path.cpp?rev=365753&r1=365752&r2=365753&view=diff
==============================================================================
--- llvm/trunk/unittests/Support/Path.cpp (original)
+++ llvm/trunk/unittests/Support/Path.cpp Thu Jul 11 03:17:59 2019
@@ -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))
More information about the llvm-commits
mailing list