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

Thu Jan 14 15:25:38 PST 2021

MaskRay added subscribers: robertu94, mgorny.
MaskRay added a comment.

In D93881#2499664 <https://reviews.llvm.org/D93881#2499664>, @jcai19 wrote:

>> But I think we should do better: instead of: (1) create a temp file (2) open it by name again (3) check it is a regular file with one hard link & it is writable (S_IWUSR) (4) fstat (5) fchown,
>> we should just modify the destination in place (for `objcopy file` and `strip file`). If the file has been mapped readonly, on many platforms it may be unwritable. We have to accept this.
>
> @Maskray I took a look at the how ELF binaries are handled and from what I understand, the code directly copies the content of the input file into temporary file (https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-objcopy/ELF/Object.cpp#L1786), before replacing the output file with it. So unless we are willing to allocate memory space to save its content (which will be very costive for large binaries), it does not appear modifying the input file in place is easy. And if we ever decide to take that path, it looks like we would have to make a lot of changes as the code of interest is buried deeply in the call hierarchy, and in addition we would likely have to change how the other file formats handle such cases to be consistent. Please let me know if I misunderstood your proposal or overlooked anything, but so far it appears to me the best place to preserve the file ownership without major change to the code is in the proposed place.

If that case, I'll personally want to rethink whether the Gentoo usage is a case we want to support directly. 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 chmod mail a.o
  sudo -u bin strip a.o  # owner becomes bin. llvm-strip has the same behavior.

You can create a wrapper script for `strip` used by root.

CC some Gentoo folks: @mgorny @robertu94

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93881/new/

https://reviews.llvm.org/D93881