[PATCH] D93881: [llvm-objcopy] preserve file ownership when overwritten

[Jian Cai via Phabricator via llvm-commits]

jcai19 created this revision.
Herald added subscribers: dexonsmith, abrachet, hiraditya, yaxunl.
Herald added a reviewer: alexshap.
Herald added a reviewer: rupprecht.
Herald added a reviewer: jhenderson.
jcai19 updated this revision to Diff 314724.
jcai19 added a comment.
jcai19 edited the summary of this revision.
jcai19 updated this revision to Diff 314795.
jcai19 published this revision for review.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Update test cases.


jcai19 added a comment.

Fix test cases to make them pass on macOS.


GNU objcopy and strip preserve the ownership of a file when it's
overwritten, e.g. while "sudo objcopy foo -o bar" creates a new file bar
with root access,  "sudo objcopy foo" will keep the owner and group of
foo unchanged. Currently llvm-objcopy and llvm-strip behave differently,
always changing the ownership to root. The discrepancy prevents Chrome
OS from migrating to llvm-objcopy and llvm-strip as Portage (the build
system) builds binaries following recommended steps documented at the
man page of GNU strip as follows,

      

1.<Link the executable as normal.>
1.<Copy "foo" to "foo.full">
1.<Run "strip --strip-debug foo">
1.<Run "objcopy --add-gnu-debuglink=foo.full foo">

      

With llvm-strip and llvm-objcopy, the last two steps change ownership of
binaries to root and cause them to become inaccessible to the intended user.

      

Link: crbug.com/1108880


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93881

Files:
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/Support/Unix/Path.inc
  llvm/test/tools/llvm-objcopy/preserve-file-ownership.test
  llvm/tools/llvm-objcopy/llvm-objcopy.cpp


Index: llvm/tools/llvm-objcopy/llvm-objcopy.cpp
===================================================================
--- llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -328,6 +328,20 @@
       return E;
   }
 
+#if !defined(_WIN32) || defined(__CYGWIN32__)
+  // Preserve the ownership if the input file is overwritten
+  if (Config.InputFilename != "-" &&
+      Config.InputFilename == Config.OutputFilename) {
+    sys::fs::file_status OStat;
+    if (std::error_code EC = sys::fs::status(Config.OutputFilename, OStat))
+      return createFileError(Config.OutputFilename, EC);
+    if (Stat.getUser() != OStat.getUser() ||
+        Stat.getGroup() != OStat.getGroup())
+      sys::fs::changeFileOwnership(Config.OutputFilename, Stat.getUser(),
+                                   Stat.getGroup());
+  }
+#endif
+
   return Error::success();
 }
 
Index: llvm/test/tools/llvm-objcopy/preserve-file-ownership.test
===================================================================
--- /dev/null
+++ llvm/test/tools/llvm-objcopy/preserve-file-ownership.test
@@ -0,0 +1,19 @@
+# RUN: yaml2obj %s -o %t
+# RUN: sudo llvm-objcopy %t
+# RUN: ls -l %t | FileCheck -DUSER=$(ls -ld %s | awk '{print $3}') -DGROUP=$(ls -ld %s | awk '{print $4}') %s
+# RUN: yaml2obj %s -o %t
+# RUN: sudo llvm-strip %t
+# RUN: ls -l %t | FileCheck -DUSER=$(ls -ld %s | awk '{print $3}') -DGROUP=$(ls -ld %s | awk '{print $4}') %s
+
+# CHECK: [[USER]] [[GROUP]]
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
Index: llvm/lib/Support/Unix/Path.inc
===================================================================
--- llvm/lib/Support/Unix/Path.inc
+++ llvm/lib/Support/Unix/Path.inc
@@ -1213,6 +1213,17 @@
   return std::error_code();
 }
 
+std::error_code changeFileOwnership(const Twine &Name, uint32_t Owner, uint32_t Group) {
+  int FD;
+  if (std::error_code EC = sys::fs::openFileForWrite(Name, FD, sys::fs::CD_OpenExisting))
+    return EC;
+  auto Chown = [&]() { return fchown(FD, Owner, Group); };
+  if ((sys::RetryAfterSignal(-1, Chown)) < 0)
+    return std::error_code(errno, std::generic_category());
+  if (std::error_code EC = sys::fs::closeFile(FD))
+    return EC;
+}
+
 } // end namespace fs
 
 namespace path {
Index: llvm/include/llvm/Support/FileSystem.h
===================================================================
--- llvm/include/llvm/Support/FileSystem.h
+++ llvm/include/llvm/Support/FileSystem.h
@@ -1159,6 +1159,17 @@
 /// means that the filesystem may have failed to perform some buffered writes.
 std::error_code closeFile(file_t &F);
 
+#ifdef LLVM_ON_UNIX
+/// @brief Change ownership of a file
+///
+/// @param Owner The owner of the file to change to
+/// @param Group The group of the file to change to
+/// @returns errc::success if successfully updated file ownership, otherwise an
+///          error code is returned.
+std::error_code changeFileOwnership(const Twine &Name, uint32_t Owner,
+                                    uint32_t Group);
+#endif
+
 /// RAII class that facilitates file locking.
 class FileLocker {
   int FD; ///< Locked file handle.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D93881.314795.patch
Type: text/x-patch
Size: 3342 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210106/ca0f5c21/attachment.bin>