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

[MaskRay]

Summary

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.

[manojgupta]

Thanks a lot Fangrui.

[jhenderson]

I am not particular up-to-speed on the file permissions stuff, so I'll defer to @manojgupta for the rest of the change.

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

35 ↗

(On Diff #325666)

I don't think this line will work. From the test comment at the top, "Windows has no umask". I don't appear to have it on my machine (which has GnuWin32 installed).

[manojgupta]

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

35 ↗

(On Diff #325666)

Yes, this is a good point about windows/umask . @MaskRay please consider removing or adjusting this test.

[MaskRay]

drop Windows test

llvm/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
 
+## 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
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64

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

if (!NewArchiveMembersOrErr)
     return NewArchiveMembersOrErr.takeError();
   return deepWriteArchive(Config.OutputFilename, *NewArchiveMembersOrErr,
                           Ar.hasSymbolTable(), Ar.kind(),
                           Config.DeterministicArchives, Ar.isThin());
 }
 
 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
   // do not set access/modification times or permissions.
   if (Filename == "-")
     return Error::success();
 
   if (auto EC =
           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);
 
   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);
 
   return Error::success();
 }
 
 /// The function executeObjcopy does the higher level dispatch based on the type
if (Archive *Ar = dyn_cast<Archive>(BinaryOrErr.get().getBinary())) {
                         Config.InputFilename == Config.OutputFilename,
                     Stat.getUser(), Stat.getGroup());
       if (Error E = executeObjcopyOnBinary(Config,
                                            *BinaryOrErr.get().getBinary(), FB))
         return E;
     }
   }
 
-  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;
   }
 
   return Error::success();
 }
 
 namespace {