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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added a comment.
This revision now requires changes to proceed.

Overall, LGTM. I am fine with fchown, but the description needs a lot of changes to proceed.

For one, the fix advice in https://reviews.llvm.org/D93881#2483542 needs to be taken, `objcopy xxx -o yyy` does not work.

You can mention that as of binutils 2.36, GNU strip calls chown(2) for `sudo strip a` and `sudo strip a -o a`, but not sudo strip a -o b or sudo strip a -o ./a.

> 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,

This part should be rephrased. I think whatever llvm-objcopy/llvm-strip resolution is, Portage may need to migrate away from in-place strip as well @mgorny @robertu94, like https://git.archlinux.org/pacman.git/commit/?id=88d054093c1c99a697d95b26bd9aad5bc4d8e170
To prevent pessimistic double write behavior with binutils 2.37.

> 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.

The question is about when root strips that is intended to have different owner/group, after atomic rename, should root restore owner/group?
I have asked many folks for their opinions. The result is contentious. It can be argued either way. In particular, I got objection like:
"the question is not 'how to justify fchown' but 'why justify fchown'" "if someone wants to sudo strip, let them do so and deal with the consequences"

I finally swing to fchown because the implicit intention to keep the file accessible by original owner/group.
The argument is stronger than "because doing this patch makes us more compatible with GNU, so we do it." While generally it makes sense to keep compatibility when I can do that for free,
the binutils behavior is quite problematic. Do we want to take its new behavior? D96308 <https://reviews.llvm.org/D96308> I have thought very hard and finally think "no".

This does mean unfortunate platform differences and slightly inconsistent behaviors, but we are going to take it.



================
Comment at: llvm/lib/Support/FileOutputBuffer.cpp:137
 #ifndef _WIN32
+  // Try to preserve file ownership if it exists.
+  if (KeepOwnership) {
----------------
if requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881