[llvm-objcopy][NFC] Move ownership keeping code into restoreStatOnFile()

[avl]

Summary

The D93881 added functionality which preserve ownership for output file
if llvm-objcopy is called under /root. That code was added into the place
where output file is created. The llvm-objcopy already has a function which
sets/restores rights/permissions for the output file.
That is the restoreStatOnFile() function. This patch moves code
(preserving ownershipping) into the restoreStatOnFile() function.

[jcai19]

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently. Also you could delete the code added in https://reviews.llvm.org/D98511 for passing user and group ID since they are no longer needed, assuming the new behavior is what we want.

[avl]

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently.

I though it would be good to keep ownership for all modes. If it is important to not doing it for split dwo file case then I will add corresponding check.

Also you could delete the code added in https://reviews.llvm.org/D98511 for passing user and group ID since they are no longer needed, assuming the new behavior is what we want.

I suppose this code:

explicit FileBuffer(StringRef FileName, bool Keep, unsigned UID, unsigned GID)
     : Buffer(FileName), KeepOwnership(Keep), UserID(UID), GroupID(GID) {}

Ok, will delete it. thanks.

[avl]

addressed comments(removed ownership keeping code from FileOutputBuffer).

[dblaikie]

In D98511#2623027, @avl wrote:

I'm not sure if this would be a big issue but restoreStatOnFile also runs on the split dwo file, and will keep its ownership with this change. I don't think GNU assembler does that currently.

I though it would be good to keep ownership for all modes. If it is important to not doing it for split dwo file case then I will add corresponding check.

I'm fine with the new behavior, if I'm understanding correctly and it means that dwo files will be treated more like/the same as how .o files have been treated by both GCC and Clang for some time now. (GCC's behavior is probably an artifact of running llvm-objcopy to make the dwo file)

[MaskRay]

The fchown behavior requires root (CAP_CHOWN) so it is not tested. I have tried several commands and the behavior is kept.

'cp' -f aa.o a.o; chmod 3777 a.o; ls -l a.o; sudo /tmp/RelA/bin/llvm-strip -p a.o; ls -l a.o
'cp' -f aa.o a.o; chmod 3707 a.o; ls -l a.o; sudo /tmp/RelA/bin/llvm-objcopy -p a.o b.o; ls -l b.o
umask 0
'cp' -f aa.o a.o; chmod 3707 a.o; ls -l a.o; sudo /tmp/RelA/bin/llvm-objcopy -p a.o b.o; ls -l b.o

Treating .dwo similar to .o is good.

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

301

/root => root.

302

The two conditions can be folded.

[avl]

Thank you for the review.

[manojgupta]

@avl Did you test this change? It has broken setuid permission preservation with sudo.

Repro case:

$ chmod +s main main
$ -rwsr-sr-x 1 manojgupta primarygroup 5936 Apr 11 17:46 main
$ sudo bin/llvm-strip main
$ ls -l main
-rwxr-xr-x 1 manojgupta primarygroup 5936 Apr 11 17:46 main

Note that setuid bit was removed.

This is causing breakage in chrome os builds :
https://bugs.chromium.org/p/chromium/issues/detail?id=1197970

Please fix or revert.

[avl]

yep, will fix or revert.

[manojgupta]

@cjdb please pick the fix once available to chrome os.

[avl]

@manojgupta @cjdb Sorry for the inconvenience. Please check, whether the problem is really fixed on your side(The commit with the fix is https://reviews.llvm.org/rGee8a5e4bc2c986b8e6c07e81fb58dc1e5a5c2d17)

[manojgupta]

Thanks for the quick turn around, I can verify that https://reviews.llvm.org/rGee8a5e4bc2c986b8e6c07e81fb58dc1e5a5c2d17 fixes the issue.

@cjdb please cherry pick to chrome os.

llvm/include/llvm/Support/FileOutputBuffer.h

 public:
   enum {
     /// Set the 'x' bit on the resulting file.
     F_executable = 1,
 
     /// Don't use mmap and instead write an in-memory buffer to a file when this
     /// buffer is closed.
     F_no_mmap = 2,
-
-    /// Preserve ownership if the file already exists.
-    F_keep_ownership = 4,
   };
 
   /// Factory method to create an OutputBuffer object which manages a read/write
   /// buffer of the specified size. When committed, the buffer will be written
   /// to the file at the specified path.
   ///
   /// When F_modify is specified and \p FilePath refers to an existing on-disk
   /// file \p Size may be set to -1, in which case the entire file is used.
   /// Otherwise, the file shrinks or grows as necessary based on the value of
   /// \p Size.  It is an error to specify F_modify and Size=-1 if \p FilePath
   /// does not exist.
   static Expected<std::unique_ptr<FileOutputBuffer>>
-  create(StringRef FilePath, size_t Size, unsigned Flags = 0,
-         unsigned UserID = 0, unsigned GroupID = 0);
+  create(StringRef FilePath, size_t Size, unsigned Flags = 0);
 
   /// Returns a pointer to the start of the buffer.
   virtual uint8_t *getBufferStart() const = 0;
 
   /// Returns a pointer to the end of the buffer.
   virtual uint8_t *getBufferEnd() const = 0;
 
   /// Returns size of the buffer.

llvm/lib/Support/FileOutputBuffer.cpp

createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {
   MemoryBlock MB = Memory::allocateMappedMemory(
       Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
   if (EC)
     return errorCodeToError(EC);
   return std::make_unique<InMemoryBuffer>(Path, MB, Size, Mode);
 }
 
 static Expected<std::unique_ptr<FileOutputBuffer>>
-createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode,
-                   bool KeepOwnership, unsigned UserID, unsigned GroupID) {
+createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
     return FileOrErr.takeError();
   fs::TempFile File = std::move(*FileOrErr);
 
 #ifndef _WIN32
-  // Try to preserve file ownership if requested.
-  if (KeepOwnership) {
-    fs::file_status Stat;
-    if (!fs::status(File.FD, Stat) && Stat.getUser() == 0)
-      fs::changeFileOwnership(File.FD, UserID, GroupID);
-  }
-
   // On Windows, CreateFileMapping (the mmap function on Windows)
   // automatically extends the underlying file. We don't need to
   // extend the file beforehand. _chsize (ftruncate on Windows) is
   // pretty slow just like it writes specified amount of bytes,
   // so we should avoid calling that function.
   if (auto EC = fs::resize_file(File.FD, Size)) {
     consumeError(File.discard());
     return errorCodeToError(EC);
#endif
   }
 
   return std::make_unique<OnDiskBuffer>(Path, std::move(File),
                                          std::move(MappedFile));
 }
 
 // Create an instance of FileOutputBuffer.
 Expected<std::unique_ptr<FileOutputBuffer>>
-FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags,
-                         unsigned UserID, unsigned GroupID) {
+FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   // Handle "-" as stdout just like llvm::raw_ostream does.
   if (Path == "-")
     return createInMemoryBuffer("-", Size, /*Mode=*/0);
 
   unsigned Mode = fs::all_read | fs::all_write;
   if (Flags & F_executable)
     Mode |= fs::all_exe;
 
FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   case fs::file_type::directory_file:
     return errorCodeToError(errc::is_a_directory);
   case fs::file_type::regular_file:
   case fs::file_type::file_not_found:
   case fs::file_type::status_error:
     if (Flags & F_no_mmap)
       return createInMemoryBuffer(Path, Size, Mode);
     else
-      return createOnDiskBuffer(Path, Size, Mode, Flags & F_keep_ownership,
-                                UserID, GroupID);
+      return createOnDiskBuffer(Path, Size, Mode);
   default:
     return createInMemoryBuffer(Path, Size, Mode);
   }
 }

llvm/tools/llvm-objcopy/llvm-objcopy.h

 struct CopyConfig;
 Expected<std::vector<NewArchiveMember>>
 createNewArchiveMembers(CopyConfig &Config, const object::Archive &Ar);
 
 /// A writeToFile helper creates an output stream, based on the specified
 /// \p OutputFileName: std::outs for the "-", raw_null_ostream for
 /// the "/dev/null", temporary file in the same directory as the final output
 /// file for other names. The final output file is atomically replaced with
-/// the temporary file after \p Write handler is finished. \p KeepOwnership
-/// used to setting specified \p UserID and \p GroupID for the resulting file
-/// if writeToFile is called under /root.
+/// the temporary file after \p Write handler is finished.
 Error writeToFile(StringRef OutputFileName,
-                  std::function<Error(raw_ostream &)> Write,
-                  bool KeepOwnership = false, unsigned UserID = 0,
-                  unsigned GroupID = 0);
+                  std::function<Error(raw_ostream &)> Write);
 
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_OBJCOPY_H

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

 #include <string>
 #include <system_error>
 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 
 Error writeToFile(StringRef OutputFileName,
-                  std::function<Error(raw_ostream &)> Write, bool KeepOwnership,
-                  unsigned UserID, unsigned GroupID) {
+                  std::function<Error(raw_ostream &)> Write) {
   if (OutputFileName == "-")
     return Write(outs());
 
   if (OutputFileName == "/dev/null") {
     raw_null_ostream Out;
     return Write(Out);
   }
 
   unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
   Expected<sys::fs::TempFile> Temp =
       sys::fs::TempFile::create(OutputFileName + ".temp-objcopy-%%%%%%", Mode);
   if (!Temp)
     return createFileError(OutputFileName, Temp.takeError());
 
-#ifndef _WIN32
-  // Try to preserve file ownership if requested.
-  if (KeepOwnership) {
-    sys::fs::file_status Stat;
-    if (!sys::fs::status(Temp->FD, Stat) && Stat.getUser() == 0)
-      sys::fs::changeFileOwnership(Temp->FD, UserID, GroupID);
-  }
-#endif
-
   raw_fd_ostream Out(Temp->FD, false);
 
   if (Error E = Write(Out)) {
     if (Error DiscardError = Temp->discard())
       return joinErrors(std::move(E), std::move(DiscardError));
     return E;
   }
   Out.flush();
static Error deepWriteArchive(StringRef ArcName,
   for (const NewArchiveMember &Member : NewMembers) {
     // For regular files (as is the case for deepWriteArchive),
     // FileOutputBuffer::create will return OnDiskBuffer.
     // OnDiskBuffer uses a temporary file and then renames it. So in reality
     // there is no inefficiency / duplicated in-memory buffers in this case. For
     // now in-memory buffers can not be completely avoided since
     // NewArchiveMember still requires them even though writeArchive does not
     // write them on disk.
-    Expected<std::unique_ptr<FileOutputBuffer>> FB = FileOutputBuffer::create(
-        Member.MemberName, Member.Buf->getBufferSize(),
-        FileOutputBuffer::F_executable | FileOutputBuffer::F_keep_ownership);
+    Expected<std::unique_ptr<FileOutputBuffer>> FB =
+        FileOutputBuffer::create(Member.MemberName, Member.Buf->getBufferSize(),
+                                 FileOutputBuffer::F_executable);
     if (!FB)
       return FB.takeError();
     std::copy(Member.Buf->getBufferStart(), Member.Buf->getBufferEnd(),
               (*FB)->getBufferStart());
     if (Error E = (*FB)->commit())
       return E;
   }
   return Error::success();
if (OStat.type() == sys::fs::file_type::regular_file) {
     if (Config.InputFilename != Config.OutputFilename)
       Perm = static_cast<sys::fs::perms>(Perm & ~sys::fs::getUmask() & ~06000);
 #ifdef _WIN32
     if (auto EC = sys::fs::setPermissions(Filename, Perm))
 #else
     if (auto EC = sys::fs::setPermissions(FD, Perm))
 #endif
       return createFileError(Filename, EC);
+
+#ifndef _WIN32
+    // Keep ownership if llvm-objcopy is called under root.
+    if (Config.InputFilename == Config.OutputFilename && OStat.getUser() == 0)
+      sys::fs::changeFileOwnership(FD, Stat.getUser(), Stat.getGroup());
+#endif
   }
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(Filename, EC);
 
   return Error::success();
 }
 
if (ProcessRaw) {
     if (!BinaryOrErr)
       return createFileError(Config.InputFilename, BinaryOrErr.takeError());
 
     if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
       if (Error E = executeObjcopyOnArchive(Config, *Ar))
         return E;
     } else {
       if (Error E = writeToFile(
-              Config.OutputFilename,
-              [&](raw_ostream &OutFile) -> Error {
+              Config.OutputFilename, [&](raw_ostream &OutFile) -> Error {
                 return executeObjcopyOnBinary(
                     Config, *BinaryOrErr.get().getBinary(), OutFile);
-              },
-              Config.InputFilename != "-" &&
-                  Config.InputFilename == Config.OutputFilename,
-              Stat.getUser(), Stat.getGroup()))
+              }))
         return E;
     }
   }
 
   if (Error E = restoreStatOnFile(Config.OutputFilename, Stat, Config))
     return E;
 
   if (!Config.SplitDWO.empty()) {