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

[MaskRay]

Summary

This fixes the EPERM error when a regular user executes llvm-objcopy a.o /dev/null

A new overload of llvm::sys::fs::setPermissions 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.

[MaskRay]

Fix unittest

[MaskRay]

Add a #ifdef _Win32 to (hopefully) fix Windows tests.
I don't know how to implement fchmod on Windows...

[abrachet]

Thanks @MaskRay, this is a big upgrade to what I had.

lib/Support/Windows/Path.inc

773 ↗

(On Diff #208121)

By no means a Windows expert, but it seems like _get_osfhandle and SetFileInformationByHandle would be a place to start?

unittests/Support/Path.cpp

1541 ↗

(On Diff #208121)

I'm not sure this test fixture is needed anymore, but it's also not hurting.

[MaskRay]

lib/Support/Windows/Path.inc

773 ↗

(On Diff #208121)

I don't even have a Windows for testing... I'll leave this to a Windows expert.

[jakehehrlich]

LGTM

[jakehehrlich]

I can't verify the windows parts today. If you can wait until tomorrow I can run it on a windows machine to test it.

[jhenderson]

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

include/llvm/Support/FileSystem.h

670 ↗

(On Diff #208122)

Doxygen comments to match the other overload?

lib/Support/Windows/Path.inc

773 ↗

(On Diff #208121)

Perhaps it's worth return a not_supported error here?

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

41 ↗

(On Diff #208122)

I feel like this needs to actually test that the permissions haven't been changed.

[abrachet]

but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

I agree with @jhenderson here. If we don't implement a setPermissions(int, perms) for Windows, I reckon we should only use setPermissions(const Twine&, perms).

lib/Support/Windows/Path.inc

773 ↗

(On Diff #208121)

We could do something hacky like this to get it to at least work on Windows.

HANDLE Handle = _get_osfhandle(FD);
char Buf[256];
GetFinalPathNameByHandleA(Handle, Buf, 256, 0);
return setPermissions(Buf, Permissions);

[MaskRay]

Address review comments

include/llvm/Support/FileSystem.h

670 ↗

(On Diff #208122)

The purpose is obvious from the function signature and the overload above. Adding another few lines of comment just clutter the code.

Actually, I think std::error_code setPermissions(const Twine &Path, perms Permissions); should just be removed if we can find an implementation of fchmod on Windows - it is very rare to set mode bits of an unknown file (with a path).

[jhenderson]

include/llvm/Support/FileSystem.h

670 ↗

(On Diff #208122)

In the source code, I agree, but if you don't add the doxygen comments, it will have none in the doxygen output, and therefore will look odd and is not so easy to switch around. Also, they have clearly different signatures so the meaning could be considered different.

Also, why would we delete the path overload? What if we don't have easy access to a FD (or don't want to try to look one up)?

lib/Support/Windows/Path.inc

778 ↗

(On Diff #208867)

make_error_code(errc::not_supported) would be a bit cleaner.

[MaskRay]

Switch to return std::make_error_code(std::errc::not_supported);

[MaskRay]

include/llvm/Support/FileSystem.h

670 ↗

(On Diff #208122)

In the source code, I agree, but if you don't add the doxygen comments, it will have none in the doxygen output, and therefore will look odd and is not so easy to switch around.

When the function does more involved tasks, I agree. Here, the function doesn't do anything smart. The comment just makes reading through the file harder.

Also, why would we delete the path overload? What if we don't have easy access to a FD (or don't want to try to look one up)?

chmod has much fewer use cases than fchmod. It is rare to chmod a foreign file. Usually, you also write/stat/read the file, then you'll need a handle. Once you have the handle, it is more natural to use fchmod (it also prevents TOCTOU race). setPermissions(Path, perms) was currently only used in one place in objcopy, I can hardly think of any future use case of it.

[jhenderson]

include/llvm/Support/FileSystem.h

670 ↗

(On Diff #208122)

When the function does more involved tasks, I agree. Here, the function doesn't do anything smart. The comment just makes reading through the file harder.

See also md5_contents, is_local, make_absolute, copy_file, and various other methods in this file, and most of them have simple behaviour. Each of them have nearly-identical doxygen comments for multiple overloads. You may disagree with this, but that is the established pattern, and makes a lot more sense when you're reading the doxygen itself. You can probably get away with a single-line comment referring to the "main" overload (i.e. the one with the comments), just noting the different parameter.

[MaskRay]

Add "Vesion of setPermissions accepting a file descriptor."

[jhenderson]

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

Thanks for the update. Can you answer these comments, please?

[MaskRay]

In D64236#1578315, @jhenderson wrote:

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?

Thanks for the update. Can you answer these comments, please?

With the current state of the code, objcopy a.o /dev/null => EPERM because it tries to set the mode bits of /dev/null (/dev/null is owned by root).
If you are root, you can even change its mode bits (usually rw-rw-rw-).

The new overload saves one stat syscall and avoids the race condition (if the filename is moved between sys::fs::openFileForWrite and sys::fs::setPermissions, the updated code will not operate on a nonexistent/different file). I'm sorry that I didn't read carefully when restoreStatOnFile was initially checked in.

[jhenderson]

Thanks, LGTM!

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

 ///       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,
-                               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.
 ///
 /// @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

 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) {
+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)
   timespec Times[2];
   Times[0] = sys::toTimeSpec(AccessTime);
   Times[1] = sys::toTimeSpec(ModificationTime);
   if (::futimens(FD, Times))
     return std::error_code(errno, std::generic_category());

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

 std::error_code status(file_t FileHandle, file_status &Result) {
   return getStatus(FileHandle, Result);
 }
 
 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;
 
   DWORD Attributes = ::GetFileAttributesW(PathUTF16.begin());
   if (Attributes == INVALID_FILE_ATTRIBUTES)
     return mapWindowsError(GetLastError());
 
std::error_code setPermissions(const Twine &Path, perms Permissions) {
   }
 
   if (!::SetFileAttributesW(PathUTF16.begin(), Attributes))
     return mapWindowsError(GetLastError());
 
   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);
   FILETIME ModifyFT = toFILETIME(ModificationTime);
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   if (!SetFileTime(FileHandle, NULL, &AccessFT, &ModifyFT))
     return mapWindowsError(::GetLastError());
   return std::error_code();

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

 # RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
 # RUN: cmp %t1.perms %t.0666
 
 # RUN: chmod 0640 %t
 # RUN: llvm-objcopy %t %t1
 # 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
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64

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

if (auto EC =
           sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))
     return createFileError(Filename, EC);
 
   if (PreserveDates)
     if (auto EC = sys::fs::setLastAccessAndModificationTime(
             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);
 
   return Error::success();
 }
 
 /// The function executeObjcopy does the higher level dispatch based on the type

llvm/trunk/unittests/Support/Path.cpp

#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*/));
+  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))
       << "Did not respect umask";
 
   (void)::umask(OldMask);
   (void)::close(FD);
 #endif