[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/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();
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)
+      if (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()) {