[llvm-objcopy] Avoid rename if input filename = output filename

[MaskRay]

Summary

In binutils, objcopy/strip perform smart_rename for "in-place" strip exe
and objcopy exe. smart_rename calls chown(2) (vulnerable:
https://sourceware.org/bugzilla/show_bug.cgi?id=26945) which requires
additional care to restore file modes and owner/group.

binutils 2.37 is moving to use truncate+overwrite
https://sourceware.org/pipermail/binutils/2021-February/115282.html

It has nice properties that owner/group/mode/security context/xattr are all
preserved. The downside is ETXTBSY (while the executable is running) and
potentially corrupted output in the case of full disk situation and other
erroneous cases.

This patch emulates its behavior when input filename = output filename
(llvm-strip exe, llvm-strip exe -o exe)[1]. llvm-objcopy/llvm-strip
treats '-' as stdin/stdout so '-' needs to be excluded from this behavior.
In addition, the implementation of --build-id-link-{input,out} (not in binutils,
contributed by Fuchsia but not in-use) needs the on-disk file so they should be excluded as well.

Changing owner/group requires privilege to test. I merely strengthen the existing file mode tests.

[1]: I'd like to just check whether -o is present, but it would need more code, and it probably does not matter to additionally make llvm-strip exe -o exe special while llvm-strip exe -o ./exe follows our original behavior.

[MaskRay]

Confirmed with phosek that --build-id-link-{input,out} is no longer used by Fuchsia, so I suspect the two options are not used by anyone now. However, I need to special case them to avoid breaking build-id-link-dir.test
Similarly, the - special case is needed to prevent breaking tests.

[kees]

Is it possible to plumb fd instead of pathname? Then fchown(), fsetxattr(), etc, can all be used?

[MaskRay]

In D96308#2551924, @kees wrote:

Is it possible to plumb fd instead of pathname? Then fchown(), fsetxattr(), etc, can all be used?

(Before this patch both input and output filenames can already be opened more than once. TOC-TOU already applies, which is bad.)

I wanted to avoid adding a new open but it is difficult. Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr = createBinary(Config.InputFilename); does not expose the file handle.
Fixing it is possible but that will be a very intrusive change.

Preserving extended attributes is possible but different OSes have different APIs, e.g. fgetxattr on Linux and extattr_get_file on FreeBSD. In the end binutils just accepts the non-atomic operation. So we just follow suit.

[jcai19]

Thanks for this patch. You mentioned the following issue in one of your comments on https://reviews.llvm.org/D93881. IIUC, this patch seems to not have taken care of this issue.

< I have mentioned that the GNU objcopy/strip smart_rename still has vulnerability and the owner preservation does not work for non-root (CAP_CHOWN) users.

< chmod o+wx .
< sudo chown mail a.o
< sudo -u bin strip a.o # owner becomes bin. llvm-strip has the same behavior.

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

334

Are there any particular reasons we need to use memory buffer instead of creating a temporary file? Will this be a problem for large binary files?

[MaskRay]

In D96308#2552376, @jcai19 wrote:

Thanks for this patch. You mentioned the following issue in one of your comments on https://reviews.llvm.org/D93881. IIUC, this patch seems to not have taken care of this issue.

< I have mentioned that the GNU objcopy/strip smart_rename still has vulnerability and the owner preservation does not work for non-root (CAP_CHOWN) users.

< chmod o+wx .
< sudo chown mail a.o
< sudo -u bin strip a.o # owner becomes bin. llvm-strip has the same behavior.

I think my description is correct. The GNU strip chown behavior is entirely about sudo strip a / sudo strip a -o a, but not sudo strip a -o b or sudo strip a -o ./a.

What this patch does is very similar to (expected) GNU binutils 2.37 behavior, not the existing binutils behavior.

I'll abandon this patch because the more I think about it, the more I value atomic operation.

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

 # RUN: umask 0
 
 # RUN: yaml2obj %s -o %t
 
 # RUN: chmod 0777 %t
 # RUN: llvm-objcopy %t %t1
 # RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
 # RUN: cmp %t1.perms %t.0777
+# RUN: llvm-objcopy %t
+# RUN: ls -l %t1 | cut -f 1 -d ' ' | cmp %t.0777 -
 
 # RUN: chmod 0666 %t
 # RUN: llvm-objcopy %t %t1
 # RUN: ls -l %t1 | cut -f 1 -d ' ' > %t1.perms
 # RUN: cmp %t1.perms %t.0666
+# RUN: llvm-objcopy %t
+# RUN: ls -l %t1 | cut -f 1 -d ' ' | cmp %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
+# RUN: llvm-objcopy %t
+# RUN: ls -l %t1 | cut -f 1 -d ' ' | cmp %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
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64

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

Expected<std::vector<NewArchiveMember>> NewArchiveMembersOrErr =
       createNewArchiveMembers(Config, Ar);
   if (!NewArchiveMembersOrErr)
     return NewArchiveMembersOrErr.takeError();
   return deepWriteArchive(Config.OutputFilename, *NewArchiveMembersOrErr,
                           Ar.hasSymbolTable(), Ar.kind(),
                           Config.DeterministicArchives, Ar.isThin());
 }
 
+static Error createOrOverwrite(StringRef Filename, ArrayRef<char> Buf) {
+  int FD;
+  std::error_code EC;
+  if (auto EC = sys::fs::openFileForWrite(
+          Filename, FD, sys::fs::CD_CreateAlways, sys::fs::OF_None,
+          sys::fs::all_read | sys::fs::all_write))
+    return errorCodeToError(EC);
+  raw_fd_ostream(FD, /*shouldClose=*/true, /*unbuffered=*/true)
+      << StringRef(Buf.data(), Buf.size());
+  return Error::success();
+}
+
 static Error restoreStatOnFile(StringRef Filename,
                                const sys::fs::file_status &Stat,
-                               bool PreserveDates) {
+                               bool PreserveDates, bool PreserveModes) {
   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 (auto EC = sys::fs::setLastAccessAndModificationTime(
             FD, Stat.getLastAccessedTime(), Stat.getLastModificationTime()))
       return createFileError(Filename, EC);
 
+  if (PreserveModes) {
     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)
 #ifdef _WIN32
       if (auto EC = sys::fs::setPermissions(
               Filename, static_cast<sys::fs::perms>(Stat.permissions() &
                                                     ~sys::fs::getUmask())))
 #else
       if (auto EC = sys::fs::setPermissions(
               FD, static_cast<sys::fs::perms>(Stat.permissions() &
-                                            ~sys::fs::getUmask())))
+                                              ~(sys::fs::getUmask() | 06000))))
 #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
case FileFormat::Binary:
     break;
   case FileFormat::IHex:
     ProcessRaw = executeObjcopyOnIHex;
     break;
   default:
     ProcessRaw = nullptr;
   }
 
+  bool PreserveModes = true;
   if (ProcessRaw) {
     auto BufOrErr = MemoryBuffer::getFileOrSTDIN(Config.InputFilename);
     if (!BufOrErr)
       return createFileError(Config.InputFilename, BufOrErr.getError());
     FileBuffer FB(Config.OutputFilename);
     if (Error E = ProcessRaw(Config, *BufOrErr->get(), FB))
       return E;
   } else {
     Expected<OwningBinary<llvm::object::Binary>> BinaryOrErr =
         createBinary(Config.InputFilename);
     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 (Config.InputFilename == Config.OutputFilename &&
+               Config.InputFilename != "-" && !Config.BuildIdLinkInput &&
+               !Config.BuildIdLinkOutput) {
+      // If input filename = output filename, create an in-memory buffer and
+      // overwrite the input instead of atomic rename to avoid owner/group/mode
+      // changes. --build-id-link-{input,output} need the on-disk file, so
+      // overwrite cannot be used.
+      MemBuffer MB(Config.OutputFilename);
+      if (Error E = executeObjcopyOnBinary(Config,
+                                           *BinaryOrErr.get().getBinary(), MB))
+        return E;
+      std::unique_ptr<WritableMemoryBuffer> WMB = MB.releaseMemoryBuffer();
+      // Release the existing handle before overwriting.
+      (void)BinaryOrErr->takeBinary();
+      if (Error E = createOrOverwrite(Config.InputFilename, WMB->getBuffer()))
+        return E;
+      PreserveModes = false;
     } else {
       FileBuffer FB(Config.OutputFilename);
       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.PreserveDates, PreserveModes))
     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.PreserveDates,
+                                    true))
       return E;
   }
 
   return Error::success();
 }
 
 namespace {