[llvm] 17b4e69 - [llvm-objcopy] If input=output, preserve umask bits, otherwise drop S_ISUID/S_ISGID bits

[Fangrui Song via llvm-commits]

Author: Fangrui Song
Date: 2021-02-24T11:10:09-08:00
New Revision: 17b4e695ce0ef89eac4a37df2df49d4c0e700766

URL: https://github.com/llvm/llvm-project/commit/17b4e695ce0ef89eac4a37df2df49d4c0e700766
DIFF: https://github.com/llvm/llvm-project/commit/17b4e695ce0ef89eac4a37df2df49d4c0e700766.diff

LOG: [llvm-objcopy] If input=output, preserve umask bits, otherwise drop S_ISUID/S_ISGID bits

This makes the behavior similar to cp

```
chmod u+s,g+s,o+x a
sudo llvm-strip a -o b
// With this patch, b drops set-user-ID and set-group-ID bits.
// sudo cp a b => b does not have set-user-ID or set-group-ID bits.
```

This also changes the behavior for the following case:

```
chmod u+s,g+s,o+x a
llvm-strip a
// a preserves set-user-ID and set-group-ID bits.
// This matches binutils<2.36 and probably >=2.37.  2.36 and 2.36.1 have some compatibility issues.
```

Differential Revision: https://reviews.llvm.org/D97253

Added: 
    

Modified: 
    llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
    llvm/tools/llvm-objcopy/llvm-objcopy.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
index 39957716a1de..8f4993f4f3d2 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test
@@ -37,12 +37,24 @@
 # RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
 # RUN: cmp %t1.perms %t.0640
 
+## Drop S_ISUID/S_ISGID bits.
+# RUN: chmod 6640 %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
 
+## Ignore umask if the output filename is the same as the input filename.
+# RUN: umask 022
+# RUN: cp %t %t1 && chmod 0777 %t1 && llvm-objcopy %t1
+# RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
+# RUN: cmp %t1.perms %t.0777
+
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64

diff  --git a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
index 42d97b2ada5a..43e3334e3317 100644
--- a/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
+++ b/llvm/tools/llvm-objcopy/llvm-objcopy.cpp
@@ -230,7 +230,7 @@ static Error executeObjcopyOnArchive(CopyConfig &Config,
 
 static Error restoreStatOnFile(StringRef Filename,
                                const sys::fs::file_status &Stat,
-                               bool PreserveDates) {
+                               const CopyConfig &Config) {
   int FD;
 
   // Writing to stdout should not be treated as an error here, just
@@ -242,7 +242,7 @@ static Error restoreStatOnFile(StringRef Filename,
           sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))
     return createFileError(Filename, EC);
 
-  if (PreserveDates)
+  if (Config.PreserveDates)
     if (auto EC = sys::fs::setLastAccessAndModificationTime(
             FD, Stat.getLastAccessedTime(), Stat.getLastModificationTime()))
       return createFileError(Filename, EC);
@@ -250,17 +250,17 @@ static Error restoreStatOnFile(StringRef Filename,
   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)
+  if (OStat.type() == sys::fs::file_type::regular_file) {
+    sys::fs::perms Perm = Stat.permissions();
+    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, static_cast<sys::fs::perms>(Stat.permissions() &
-                                                  ~sys::fs::getUmask())))
+    if (auto EC = sys::fs::setPermissions(Filename, Perm))
 #else
-    if (auto EC = sys::fs::setPermissions(
-            FD, static_cast<sys::fs::perms>(Stat.permissions() &
-                                            ~sys::fs::getUmask())))
+    if (auto EC = sys::fs::setPermissions(FD, Perm))
 #endif
       return createFileError(Filename, EC);
+  }
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return createFileError(Filename, EC);
@@ -320,14 +320,12 @@ static Error executeObjcopy(CopyConfig &Config) {
     }
   }
 
-  if (Error E =
-          restoreStatOnFile(Config.OutputFilename, Stat, Config.PreserveDates))
+  if (Error E = restoreStatOnFile(Config.OutputFilename, Stat, Config))
     return E;
 
   if (!Config.SplitDWO.empty()) {
     Stat.permissions(static_cast<sys::fs::perms>(0666));
-    if (Error E =
-            restoreStatOnFile(Config.SplitDWO, Stat, Config.PreserveDates))
+    if (Error E = restoreStatOnFile(Config.SplitDWO, Stat, Config))
       return E;
   }